feat(kotlin-sdk)!: keystore rework — policy-alias split, layered key recovery, durable repair, structured signer errors (stacked on #4191)#4183
Open
bfoss765 wants to merge 27 commits into
Conversation
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 21, 2026 12:40
Contributor
📝 WalkthroughWalkthroughAdds policy-based keystore aliases and migration handling, transactional pending identity-key tracking, structured signer errors, typed platform-wallet errors, and managed-identity lookup classification across Kotlin, Rust, and Swift. ChangesKeystore policy and identity-key recovery
Pending identity-key repair state
Signer and platform-wallet error flows
Schema and parity artifacts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Signer
participant KotlinJNI
participant RustFFI
participant PlatformWallet
Signer->>KotlinJNI: completeSign(errorCode, errorMessage)
KotlinJNI->>RustFFI: forward structured signer error
RustFFI->>PlatformWallet: map signing-key-unavailable prefix to code 31
PlatformWallet-->>Signer: return typed platform-wallet error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
⛔ Blockers found — Sonnet deferred (commit 0d28d5e) |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- Around line 549-562: Update the migration block around store.edit in
WalletStorage so CancellationException is not swallowed by runCatching; rethrow
cancellation while retaining the existing handling for non-cancellation failures
and successful migration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 987bcd6c-12ec-4a70-8f81-bdad970986df
📒 Files selected for processing (15)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreKeyGenPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageUpgradeMatrixTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.ktpackages/rs-platform-wallet-ffi/src/dashpay.rs
Collaborator
|
This needs reconciliation with #4172 before it can merge — the branch is
Suggestion: split the unmanaged-identity-reads + typed |
Collaborator
|
Additional findings after checking the existing threads and the current #4172 base:
I am not repeating the existing rebase/invalidation-recovery and cancellation comments; those remain valid. The earlier |
Contributor
Author
|
Took your suggestion: the conflict-free slices are split out as #4191 — unmanaged-identity reads (including both |
bfoss765
force-pushed
the
port/v4.1/android-integration
branch
from
July 21, 2026 23:26
00804cd to
ce3490a
Compare
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 21, 2026
…as split The inherited dashpay#4172 instrumented tests hardcode the legacy KEYS_ALIAS and the single-alias API; after the alias split the default-policy storage writes under KEYS_ALIAS_AUTH_GATED, so simulating keypair replacement by deleting the legacy alias no longer invalidates anything (the CI failure shumkov diagnosed on storeIfAbsentRederivesWhenTheKeysAliasKeypairWasReplaced). - WalletStorageOwnershipTest: the replaced-keypair, rejected-blob, and no-regeneration probes delete/inspect KEYS_ALIAS_AUTH_GATED; the fingerprint-capture test uses encryptForIdentityKeysAlias (and pins the captured producing alias); the stale-invalidation-cleanup test uses the alias-parameterized deleteIdentityKeysAliasIfCurrentGeneration. - KeystoreSignerInstrumentedTest: canSignWith rejection simulates replacement of the policy alias. The five DashPayUnlockAndSyncTest / WalletManagerRoundTripTest failures from PR dashpay#4183's CI run were NOT test or SDK defects: the branch predated dashpay#4172's workflow hardening (screen_off_timeout / stayon / dismiss-keyguard + the hard deviceLocked=0 guard), so the emulator re-locked mid-run and every MASTER_ALIAS operation threw InvalidKeyException. Rebuilding on the current base inherits the fixed workflow; those tests are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
changed the title
bfoss765
changed the title
Collaborator
|
Consolidated re-review of the rework (two independent passes). First the good news: all seven previously-blocking items are genuinely resolved — invalidated-key cleanup survives with generation-checked deletion, the retrieve ladder layers legacy shape-dispatch before the fingerprint gate, However, the rework surfaces new blocking findings:
|
Contributor
Author
|
All five new findings addressed (255 unit tests, 0 failures):
Two honest residuals, both fail-closed (neither can persist a wrong key): the real deriver's FFI pubkey-equality is exercised on-device rather than in a JVM unit test (AndroidKeyStore crypto can't run on the JVM) — it's in our device test plan; and on API 29–32 the numeric error code isn't available, so an exotic "reports-secure-but-rejects" OEM quirk on old Android wouldn't be text-classified, though genuine lockless devices are still caught by the primary |
…otFound Split out of dashpay#4183 (port of dashpay#4060) per review: the unmanaged-identity read fixes are conflict-free on v4.1-dev and should not be blocked on the Keystore rework. The FFI's blanket Option -> result conversion reports an identity the wallet does not manage as PlatformWalletFFIResultCode::NotFound (98), so the Kotlin callers' zero-handle checks were dead and every local read over an unmanaged identity threw instead of returning absence. - Dashpay: route getManagedIdentity through translateManagedIdentityNotFoundToZero so contacts()/syncState()/ payments()/sendContactRequest() treat "not managed" as null/empty/ false; ErrorInvalidHandle still propagates. - Sweep the remaining dead `== 0L` sites: ManagedPlatformWallet .inMemoryIdentityStates (one unmanaged/just-removed id no longer throws through the whole listing) and IdentityRegistration .contestedDpnsNames (the intended "identity is not managed by this wallet" NotFound now actually surfaces). - platform-wallet-ffi: platform_wallet_get_managed_identity keeps the three outcomes distinct (classify_managed_identity_outcome) so a stale/removed wallet surfaces ErrorInvalidHandle and never masquerades as an unmanaged identity; unit tests pin both arms. - DashSdkError: name the code (PLATFORM_WALLET_NOT_FOUND_CODE = 98); the 98 -> DashSdkError.NotFound mapping is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Split out of dashpay#4183 (port of dashpay#4060) per review: the typed signing error is conflict-free on v4.1-dev and should not be blocked on the Keystore rework. The KeystoreSigner "missing key" completion error travels as free text through Rust and used to come back as an opaque PlatformWallet.Generic (or WalletOperation) failure. It is now built from a shared MESSAGE_MARKER constant and recognized on the Kotlin boundary as the typed DashSdkError.PlatformWallet.SigningKeyUnavailable, so hosts can route users to key repair instead of showing a generic error. The marker is only consulted on the catch-all codes (6 / else), so the dedicated retry-semantics types are never overridden. Known limitation (kept as-is from dashpay#4183 by request): the discriminator is message-text-based (message.contains on the marker); a structured error code across the FFI boundary is follow-up work in the parent PR line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduce KeySecurityPolicy (AUTH_GATED default / DEVICE_BOUND opt-in) and move new identity-key wrapping off the single KEYS_ALIAS onto two dedicated RSA aliases, one per policy — Keystore auth parameters are fixed at key generation, so the policies can never share an alias. The legacy KEYS_ALIAS becomes read-only: it retains whichever superseded key (auth-gated AES-GCM, or the pre-split RSA pair) an upgraded install still holds, reachable only through the decryptLegacy* migration fallbacks. The dashpay#4172 hardening survives the split alias-parameterized: write-time fingerprint capture (encryptForIdentityKeysAlias — one lookup for blob + fingerprint + producing alias), the non-generating fingerprint read, and the KeyPermanentlyInvalidatedException recovery with its generation-checked, per-alias deletion (deleteIdentityKeysAliasIfCurrentGeneration, which refuses the legacy alias by contract). WalletStorage grows the policy-taking constructor and routes identity-key writes/reads through the policy alias. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…und alias, honestly The wallet must work without a screen lock (dashpay#4060), but the AUTH_GATED authentication gate cannot exist without one — KeyMint rejects generate_key. Instead of silently generating a gate-less key under the auth-gated alias (a policy lie), identity-key writes on a lockless device are redirected to KEYS_ALIAS_DEVICE_BOUND, whose gate-less parameters are inherent: - encryptForIdentityKeys resolves the effective write alias (probe first, KeyMint safety-net rejection second) and returns the producing alias with the blob; WalletStorage records it as a per-blob privkeyalias. tag written atomically with the blob + fingerprint, and reads decrypt under the RECORDED alias — keys written while lockless stay readable after a lock screen is enrolled and new writes move to the gated alias. A missing tag means the policy alias (backward compatible with earlier data). - effectiveKeySecurityPolicy() (KeystoreManager + WalletStorage) surfaces the degradation; PlatformWalletManager logs it once at construction. - Strict mode: KeystoreManager(requireAuthGated = true) throws the new KeySecurityPolicyUnavailableException instead of degrading. - MASTER_ALIAS (AES) and the DEVICE_BOUND alias keep the in-place setUnlockedDeviceRequired degradation (no auth gate to lie about); the auth-gated alias is never routed through it. - isNoSecureLockScreenKeyGenFailure is tightened: a nested KeyStoreException classifies only inside the key-gen ProviderException's cause chain AND with a lock-screen message or the observed rejection's numeric code (internal Keystore 4 / KeyMint 10309, read reflectively) — a bare nested Keystore error with an unrelated message no longer triggers the retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bing capability split retrievePrivateKey becomes an explicit three-rung ladder that composes the dashpay#4172 fingerprint gate with the legacy-blob recovery paths: 1. Legacy AES-GCM blobs shape-dispatch FIRST — the write-time fingerprint gate never applies to them (they predate it or carry a superseded key's fingerprint); the retained legacy KEYS_ALIAS AES key recovers the value, which is migrated to the policy alias. 2. Empty-IV RSA blobs whose stored fingerprint matches the RECORDED alias's current key (non-generating read, alias-tag routed) decrypt on the fast path. KeyPermanentlyInvalidatedException rethrows after KeystoreManager's generation-checked cleanup — after the deletion the fingerprint can no longer match, so reads and repair re-DERIVE rather than trusting a stale certificate (closes the invalidation brick loop). Other crypto failures fall through (defense in depth). 3. A mismatching or MISSING fingerprint is a routing signal into the former-KEYS_ALIAS-RSA recovery ladder, not an immediate null — pre-split blobs carry the former key's fingerprint and must reach decryptLegacyRsaKeysBlob. The policy alias is never touched here, and a read never provisions a keypair. Capability is split per finding 3: isPrivateKeyDecryptable stays the CHEAP check (presence + fingerprint only — the Rust canSignWith callback thread must never decrypt, prompt, or generate), extended with presence rungs for the legacy schemes; the real-decrypt health check moves to the new probeIdentityKeyRecoverability (DEVICE_BOUND-sibling disproof included, skipped for blobs the sibling legitimately owns by tag), which the example app's WalletKeyHealthSheet now calls. UserNotAuthenticatedException propagates from every rung — a closed auth window is never a wrong key. WalletStorageUpgradeMatrixTest pins every ladder row through the fake Keystore seam, including the new invalidation-cleanup, alias-tag-routing, fast-path-failure, and never-decrypts-on-the-cheap-path rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…key failures Port of the dashpay#4053 pending-keys machinery: when the identity-key persist callback's derive/store fails, the key lands in Room watch-only and every signature with it will fail — previously a single Log.w was the only trace. PlatformWalletPersistenceHandler now records a PendingIdentityKey entry (wallet/identity/keyId, derivation breadcrumbs, reason, timestamp) in a StateFlow keyed by public-key hex, exposed through PlatformWalletManager.pendingIdentityKeys so hosts can surface a repair path. Transactional with the changeset round (finding de3cf44a71fc): while a store round is open, record/clear mutations are staged as pure map deltas in the round's ChangesetBuffer and published in ONE atomic MutableStateFlow.update only after the Room transaction commits; a rolled-back or aborted round discards them with the buffer, leaving the pre-round map untouched. Standalone upserts publish immediately, as does markIdentityKeyRepaired — which repairIdentityKey now calls on success, since the repair stores the scalar through the deriver and bypasses the only persist path that clears pending entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rt (Room 7→8) pendingIdentityKeys was process-lifetime only: the durable public_keys row kept just privateKeyKeychainIdentifier = null, dropping the derivation breadcrumbs a repair needs — kill the app and the repair signal was gone. - Room migration 7→8 adds nullable public_keys.derivationIdentityIndex / derivationKeyIndex. Room is the durability substrate deliberately: the wallet-deletion cascade removes these rows, so pending entries die with their wallet automatically (a DataStore side-table would leak). - onPersistIdentityKeyUpsert records the breadcrumbs whenever Rust supplied derivation indices — success AND failure paths (the breadcrumb is not a failure marker; the null identifier is). - reconstructPendingIdentityKeysFromPersistence rebuilds the map on launch: a breadcrumbed, non-read-only row re-seeds pending when its identifier is null OR the stored blob fails the CHEAP capability check — the second disjunct resurrects repair slots for blobs stranded by a Keystore keypair replacement, not just never-derived keys. One atomic update, live entries never overwritten, reason = "reconstructed from persistence after restart". PlatformWalletManager.loadPersistedWallets runs it before the manager is handed to the host. - DashDatabaseMigrationTest gains the 7→8 case (NULL for pre-existing rows, explicit values accepted); the 4→latest and 1→latest chains extend to 8; schema 8.json exported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t.NotFound PlatformWalletFFIResultCode::NotFound (98) — the blanket Option→result miss (unknown wallet id, unmanaged identity, …) — previously collapsed into the top-level DashSdkError.NotFound shared with rs-sdk-ffi codes 7/8. It now maps to the new DashSdkError.PlatformWallet.NotFound, keeping 98 inside the wallet-error family in exact parity with Swift's PlatformWalletError.notFound, so callers can match a wallet-level absence without sniffing Generic codes. BREAKING: Kotlin hosts catching DashSdkError.NotFound from platform-wallet operations (e.g. rescanSpvFilters with an unknown wallet id) now receive DashSdkError.PlatformWallet.NotFound; rs-sdk-ffi codes 7/8 still map to the top-level NotFound. Dashpay's managed-identity local reads are unaffected — translateManagedIdentityNotFoundToZero intercepts the RAW code (dashpay#4051) before this mapping ever runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…as split The inherited dashpay#4172 instrumented tests hardcode the legacy KEYS_ALIAS and the single-alias API; after the alias split the default-policy storage writes under KEYS_ALIAS_AUTH_GATED, so simulating keypair replacement by deleting the legacy alias no longer invalidates anything (the CI failure shumkov diagnosed on storeIfAbsentRederivesWhenTheKeysAliasKeypairWasReplaced). - WalletStorageOwnershipTest: the replaced-keypair, rejected-blob, and no-regeneration probes delete/inspect KEYS_ALIAS_AUTH_GATED; the fingerprint-capture test uses encryptForIdentityKeysAlias (and pins the captured producing alias); the stale-invalidation-cleanup test uses the alias-parameterized deleteIdentityKeysAliasIfCurrentGeneration. - KeystoreSignerInstrumentedTest: canSignWith rejection simulates replacement of the policy alias. The five DashPayUnlockAndSyncTest / WalletManagerRoundTripTest failures from PR dashpay#4183's CI run were NOT test or SDK defects: the branch predated dashpay#4172's workflow hardening (screen_off_timeout / stayon / dismiss-keyguard + the hard deviceLocked=0 guard), so the emulator re-locked mid-run and every MASTER_ALIAS operation threw InvalidKeyException. Rebuilding on the current base inherits the fixed workflow; those tests are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity, and Room v8 shift - sdk-parity-manifest.json (+ regenerated PARITY_SUMMARY.md): new capability entries for the KeySecurityPolicy alias split (Swift not-applicable — iOS Keychain has no per-alias auth-parameter analog), the now-CONVERGENT platform-wallet code-98 typed mapping (was a divergence), the Kotlin-only durable pending-repair surface (Swift gap recorded), and the structured SigningKeyUnavailable discriminator (both hosts, unit-verification pointers on each). shared_symbols gains dash_sdk_sign_async_completion. - KOTLIN_SWIFT_SHARED_PARITY_SPEC.md: rationale for each divergence / convergence, the deprecated MESSAGE_MARKER fallback with its next-minor-release removal horizon, and the migration ladder update (v8 = derivation breadcrumbs; invitations shift to v9). - KOTLIN_MIGRATION_LEFTOVERS.md: marker-fallback removal and the on-device KeyPermanentlyInvalidatedException residual (unit-tier only via the fake seam, same as dashpay#4172). - Swift: testPlatformWalletNotFoundFFIResultMapping pins the convergent 98 mapping the manifest references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…closed auth window as recoverable probeIdentityKeyRecoverability treated UserNotAuthenticatedException as "recoverable" unconditionally. UNAE is thrown at cipher.init — before the ciphertext is examined — so after a Keystore loss + regeneration the fresh auth-gated key (whose window is closed in the steady state; it is only open ~30 s after an auth) reported a blob it can never open as healthy: the key-health sheet offered no repair while pendingIdentityKeys simultaneously listed the same key (dashpay#4060 round-2 finding 1). Prompt-free fix: UNAE counts as recoverable ONLY while the blob's stored write-time fingerprint still matches the recorded alias's CURRENT key (non-generating read). A mismatch disproves ownership → not recoverable → repair is offered; a fingerprint-matched blob behind a closed window stays recoverable (genuinely just needs auth). The legacy former-RSA rung keeps the documented residual (no fingerprint surface for the retained legacy key). New matrix rows pin both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ped signer code on the first attempt A KPIE escaping the sign path (retrieveKeyWithAuth — the biometric-gate retry only handles UserNotAuthenticatedException) previously fell into the signAsync catch-all and completed with SIGNER_ERROR_CODE_GENERIC: the host saw an opaque wallet-operation failure on the FIRST post-invalidation sign and only got the typed SigningKeyUnavailable on subsequent attempts (once the fingerprint gate reported the blob non-current). dashpay#4060 round-2 finding 2. - signWithStoredKey catches KPIE explicitly and completes with SIGNER_ERROR_CODE_KEY_UNAVAILABLE (marker text + invalidation reason kept in the message), so the typed code rides the wire immediately. - The signAsync catch-all classifies via the new pure completionErrorCodeFor(t) (KPIE → key-unavailable, everything else generic) as belt-and-braces for KPIE thrown outside the retrieval call. - KeystoreSignerCompletionCodeTest pins the classification (the full signer cannot be constructed on the JVM — its constructor creates a native handle — so the classification is a pure companion function). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… for legacy-alias keys A KeyPermanentlyInvalidatedException on a LEGACY-alias-backed key was invisible everywhere: the legacy Keystore aliases are read-only (no deletion boundary), so hasLegacyKeysKey() stays true forever — canSignWith keeps offering the key, the cheap capability check keeps reporting the blob usable, the restart reconstruction never seeds a repair slot, and every sign fails opaquely (dashpay#4060 round-2 finding 3). Fix without making the cheap check expensive (it is untouched): the sign path's KPIE classification now invokes an onSigningKeyInvalidated hook, wired by PlatformWalletManager to PlatformWalletPersistenceHandler.recordSigningKeyInvalidated, which nulls privateKeyKeychainIdentifier on the key's Room rows — the same durable signal the restart reconstruction reads — and re-runs the reconstruction so pendingIdentityKeys seeds after the FIRST failed sign (reason: "signing key permanently invalidated"), making repair reachable outside the health sheet. Best-effort: bookkeeping failure never eats the typed completion. Harmless for policy-alias keys (their generation-checked deletion already flips the fingerprint gate; this just accelerates the in-process seed). Pre-v8 rows without breadcrumbs still get the identifier null-out and seed once the next persist round back-fills their breadcrumbs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g-2 TOCTOU window Round-2 nits (dashpay#4060): - The MESSAGE_MARKER fallback's stated rationale — "old native library + new Kotlin partial builds" — was not viable: the sign-completion JNI arity change (3→4 args) makes that pairing a type-confused native call on every completion, so mixed artifacts are unsupported outright. The fallback's real purpose is the dashpay#4191 merge-order transition (whose marker-based classification predates the typed code 31) plus defense in depth for conversion paths that lose the machine prefix. KDoc/comments in DashSdkError, KeystoreSigner, DashSdkErrorTest, and the parity/leftovers docs now say so; removal horizon unchanged (next minor release). - retrievePrivateKey rung 2 gains a comment naming the accepted fingerprint-read→decrypt TOCTOU window (pre-existing dashpay#4172 parity): a concurrent rotation fails as a wrong-key crypto error into the recovery ladder — never stale plaintext. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…former-RSA rung
Round-3 verification finding: tryFormerRsaRecovery collapsed every
GeneralSecurityException to null ('not this key'), which swallowed a
KPIE from an invalidated pre-alias-split key — the signer's classifier
never saw it, so the typed completion and the durable repair seeding
(finding-3 hook) never fired for legacy blobs. KPIE now propagates like
UserNotAuthenticatedException, with a matrix row pinning the rethrow and
the post-regen recovery path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s and verify the pubkey before persisting Blocker 1 (shumkov): the repair path derived from caller-supplied indices (the example app passed the DPP key id as keyIndex), and its only check was probeIdentityKeyRecoverability — which proves the stored blob DECRYPTS, not that the derived key is the RIGHT one. A wrong index derives a different valid scalar that round-trips fine, clears pending state, and persists an unusable key. Fix: - IdentityKeyPrivateKeyDeriver: on the force (repair) path derive the KEYPAIR and verify the derived public key equals publicKeyData BEFORE any store; a mismatch throws IdentityKeyDerivationMismatchException without persisting. - PlatformWalletPersistenceHandler.repairIdentityKeyDurably: new orchestration that reads the derivation slot from the PERSISTED public_keys breadcrumbs (derivationIdentityIndex/derivationKeyIndex), never a caller index; a row without breadcrumbs fails the repair without clearing pending. Hoisted here (not the JVM-unconstructable manager) so it is unit-testable and shares the authoritative pendingIdentityKeys state. - PlatformWalletManager.repairIdentityKey: drop the identityIndex/keyIndex params and delegate; supply only the wallet-scoped probe. - WalletKeyHealthSheet: stop passing report.identityIndex / key.keyId. Tests (PlatformWalletPersistenceHandlerTest): correct breadcrumbs derive, verify, clear pending, and record the durable identifier (deriver called with the persisted 3/5, not a caller index); mismatched breadcrumbs are rejected (mismatch exception), nothing persisted, blob never probed, pending intact; a row without breadcrumbs fails with SigningKeyUnavailable and never derives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Blocker 3 (shumkov): in the repair path the Room privateKeyKeychainIdentifier update was wrapped in runCatching (swallowed on failure) while the pending state was cleared regardless. A failed durable write then resurrected the repair after restart (the reconstruction reads the persisted rows) while the live session believed it was done — session and restart disagreed. Fix: the durable write in repairIdentityKeyDurably now fails CLOSED — it runs through an injectable persistDurableIdentifier seam (default: the production public_keys write) and its exception propagates; markIdentityKeyRepaired only runs after the write commits. A failed durable write leaves pending intact and the repair retryable, so the session and a restart agree. Test: a repair whose derive + verify succeed but whose durable write throws propagates the failure and leaves the pending entry in place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…horitative signals Blocker 2 (shumkov): isNoSecureLockScreenKeyGenFailure classified any KeyStoreException whose message contained "generate_key" as a lock-screen rejection. But "generate_key" names the failing operation, not its cause — a transient KeyMint generation failure on a device WITH a lock screen carries it too, so a transient failure would silently and PERMANENTLY downgrade an AUTH_GATED key to DEVICE_BOUND instead of retrying. Fix: drop the bare "generate_key" substring match. Classification now rests only on the two authoritative signals — explicit lock-screen text, or the lock-screen rejection numeric code (internal Keystore 4 / KeyMint 10309, which the real android.security.KeyStoreException exposes via getNumericErrorCode()). Tests: the on-device shape now classifies via the numeric code (not the incidental generate_key text); two new negatives — a bare generate_key failure, and a transient generate_key failure under a key-gen ProviderException with a non-lock-screen numeric code — must NOT classify, so AUTH_GATED is retried, not downgraded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…olicy-alias migration Finding 4 (shumkov): migrateToPolicyAlias wrapped its encrypt + DataStore edit in runCatching, which swallows kotlin CancellationException along with genuine rewrite failures. A coroutine cancelled during the migration's suspend points was silently absorbed instead of unwinding — a structured-concurrency violation. Fix: replace runCatching with an explicit try/catch that rethrows CancellationException; only genuine rewrite failures stay best-effort (the recovered value is still returned; migration retries on the next read). Test: a cancellation landing at the migration encrypt (via the fake keystore's onNextPolicyEncrypt hook) now propagates out of retrievePrivateKey rather than being swallowed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fallback Finding 5 (shumkov): the DEVICE_BOUND documentation promised hardware-backed storage while the implementation permits a software AndroidKeyStore fallback (generation prefers StrongBox, falls back to the TEE, and finally to a software-backed AndroidKeyStore key on devices with no secure element — generateWithLockScreenDegradation never fails generation on a missing one). Align the docs: "device-bound" means non-exportable AndroidKeyStore, not a hardware-storage guarantee; backing is hardware-isolated only where the device provides it. AUTH_GATED shares the same backing characteristics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
force-pushed
the
port/v4.1/android-integration
branch
from
July 23, 2026 01:10
8313192 to
48c4726
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt (1)
1001-1005: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
runCatchingaround a suspend call swallowsCancellationException.reconstructPendingIdentityKeysFromPersistence(...)is asuspendfunction;runCatchingcatchesThrowable, so ifloadPersistedWalletsis cancelled while this best-effort reconstruction is suspended, the cancellation is absorbed and the coroutine keeps going into the wallet-restore loop instead of unwinding. Given this PR already tightenedCancellationExceptionhandling elsewhere, rethrow it here too.♻️ Rethrow cancellation, swallow the rest
- runCatching { - persistenceHandler.reconstructPendingIdentityKeysFromPersistence( - isPrivateKeyDecryptable = { walletStorage.isPrivateKeyDecryptable(it) }, - ) - } + try { + persistenceHandler.reconstructPendingIdentityKeysFromPersistence( + isPrivateKeyDecryptable = { walletStorage.isPrivateKeyDecryptable(it) }, + ) + } catch (c: kotlinx.coroutines.CancellationException) { + throw c + } catch (_: Throwable) { + // Best-effort: a reconstruction failure must not block wallet restore. + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt` around lines 1001 - 1005, Update the runCatching block around reconstructPendingIdentityKeysFromPersistence to rethrow CancellationException while continuing to swallow other failures. Preserve the existing best-effort behavior for non-cancellation exceptions and allow coroutine cancellation to propagate before the wallet-restore loop continues.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.kt`:
- Around line 94-102: Update the state passed from the probe block around
probeIdentityKeyRecoverability to represent key recoverability rather than
keystore-entry presence, and rename the corresponding property and usages in
KeyRow. Preserve the false result for failed probes, but render that state as an
unrecoverable/decryption failure instead of “no Keystore entry,” while retaining
the existing message for genuinely absent entries.
- Around line 94-102: Move the synchronous recoverability probing in the
key-health report producer off the composition thread by wrapping the
report-building work, including probeIdentityKeyRecoverability, in
withContext(Dispatchers.IO), then assign the completed report to produceState’s
value. Keep the existing probe results and fallback behavior unchanged.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- Around line 1001-1005: Update the runCatching block around
reconstructPendingIdentityKeysFromPersistence to rethrow CancellationException
while continuing to swallow other failures. Preserve the existing best-effort
behavior for non-cancellation exceptions and allow coroutine cancellation to
propagate before the wallet-restore loop continues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d0971c9-bcbb-40ff-8147-c9ef5050c3d3
📒 Files selected for processing (50)
docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.mddocs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.mddocs/sdk/sdk-parity-manifest.jsonpackages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.ktpackages/kotlin-sdk/PARITY_SUMMARY.mdpackages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/8.jsonpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabaseMigrationTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/WalletStorageOwnershipTest.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/SignerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/PublicKeyDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PublicKeyEntity.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/IdentityKeyPrivateKeyDeriver.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyUnavailableException.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreKeyGenPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerCompletionCodeTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageUpgradeMatrixTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.ktpackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-sdk-ffi/src/signer.rspackages/rs-sdk-ffi/src/test_utils.rspackages/rs-sdk-ffi/src/token/claim.rspackages/rs-sdk-ffi/src/token/config_update.rspackages/rs-sdk-ffi/src/token/destroy_frozen_funds.rspackages/rs-sdk-ffi/src/token/emergency_action.rspackages/rs-sdk-ffi/src/token/freeze.rspackages/rs-sdk-ffi/src/token/mint.rspackages/rs-sdk-ffi/src/token/purchase.rspackages/rs-sdk-ffi/src/token/set_price.rspackages/rs-sdk-ffi/src/token/transfer.rspackages/rs-sdk-ffi/src/token/unfreeze.rspackages/rs-unified-sdk-jni/src/signer.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift
🚧 Files skipped from review as they are similar to previous changes (36)
- packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift
- packages/rs-sdk-ffi/src/test_utils.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/SignerNative.kt
- packages/rs-sdk-ffi/src/token/emergency_action.rs
- packages/rs-sdk-ffi/src/token/set_price.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/PublicKeyDao.kt
- packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.kt
- packages/rs-sdk-ffi/src/token/claim.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyUnavailableException.kt
- packages/rs-sdk-ffi/src/token/config_update.rs
- packages/rs-sdk-ffi/src/token/transfer.rs
- packages/rs-sdk-ffi/src/token/unfreeze.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
- packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.kt
- docs/sdk/sdk-parity-manifest.json
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt
- packages/rs-sdk-ffi/src/token/mint.rs
- packages/rs-unified-sdk-jni/src/signer.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.kt
- packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerCompletionCodeTest.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PublicKeyEntity.kt
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift
- packages/kotlin-sdk/PARITY_SUMMARY.md
- packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.kt
- packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/WalletStorageOwnershipTest.kt
- packages/rs-platform-wallet-ffi/src/dashpay.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt
- packages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/8.json
- packages/rs-sdk-ffi/src/signer.rs
- packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageUpgradeMatrixTest.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt
Collaborator
|
Round-3 verification (two independent passes, reconciled): all five previous findings are genuinely fixed — breadcrumb-driven repair with the index parameters removed from the API entirely, fail-closed durable writes, derived-pubkey verification before persistence, narrowed classifier, cancellation rethrow, honest DEVICE_BOUND docs. Two new findings from the deeper pass:
Nit: several new comments carry fix-round narration (issue/finding/reviewer provenance) — keep the invariant text, move the provenance to the PR description. |
…re (dashpay#4183) The repair path derived the KEYPAIR and required its public half to equal the key's stored on-chain data before persisting. For ECDSA_HASH160 / EDDSA_25519_HASH160 keys DPP stores the 20-byte HASH160 of the pubkey as that data, not the pubkey, so the raw 33-vs-20-byte contentEquals could never match and those key types were permanently un-repairable. derivedPublicKeyMatches now takes the DPP key-type discriminant and HASH160s the derived pubkey (RIPEMD160(SHA256)) before comparing for HASH160 types; every other type keeps the plain content comparison. keyType is threaded through PrivateKeyDeriver.deriveAndStore; the repair path reads it from the persisted row's breadcrumbs, the store path passes it from the persist callback. Adds a pure-Kotlin Hash160 helper (public bytes only — no derivation/secrets, within the CLAUDE.md doctrine) pinned to RIPEMD-160 reference vectors, and tests proving a HASH160-type key repairs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dashpay#4183) LOCK_SCREEN_KEYGEN_REJECTION_CODES included android.security.KeyStoreException code 4, but 4 is ERROR_INTERNAL_SYSTEM_ERROR — a generic/transient fault, not the no-secure-lock-screen signal (code 3). Classifying it as no-lock-screen silently and permanently downgraded an AUTH_GATED identity key to the weaker DEVICE_BOUND alias on a transient error. Keep only the KeyMint-specific 10309 in the numeric set. A genuine transient internal error now falls through and is rethrown by resolveIdentityKeysWriteAlias as a retryable write failure instead of a security downgrade; the explicit lock-screen message path still classifies real no-LSKF rejections. Adds a negative test that code 4 does not classify, and repoints the existing numeric-code tests at 10309. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ancellation (dashpay#4183) onSigningKeyInvalidated wrapped the durable recordSigningKeyInvalidated write in a bare runCatching {} that swallowed a failed OR cancelled write while the sign still returned typed code 31 — the pending-repair signal was lost silently and a cancelled signer scope was masked. Replace with a try/catch that rethrows CancellationException and, on other failures, logs loudly and rethrows so the signer's own best-effort guard (not this lambda) is the single place that treats bookkeeping failure as non-fatal; the repair stays retryable (durable rows are untouched; the next sign attempt / next load reconstruction re-runs it). Also fix loadPersistedWallets: its bare runCatching around the suspend reconstructPendingIdentityKeysFromPersistence swallowed CancellationException — rethrow it (a best-effort reconstruction failure is still absorbed and logged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ashpay#4183) The WalletKeyHealthSheet recoverability probes (which decrypt each Keystore blob) ran inside produceState on the composition/Main thread; wrap the whole report build in withContext(Dispatchers.IO). The state was named hasPrivateKey and every failed probe was labeled "Missing — no Keystore entry", but the probe can't tell a truly-absent key from a present- but-stranded/undecryptable one. Rename to isRecoverable and relabel ("Unrecoverable — key material missing or stranded; re-derive to repair", summary "Unrecoverable key material") so the UI reflects recoverability, not presence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 24, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The keystore alias split and durable repair plumbing are substantial, but three in-scope correctness blockers remain. Lock-screen removal can route new writes to an invalidated auth-gated key, cancellation is swallowed inside pending-key reconstruction, and deletion paths leave stale pending-key state; the signer error conversion also allows generic messages to be misclassified as typed key-unavailable failures.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:166-169: Lock-screen removal still reuses the auth-gated alias
The auth-gated alias presence check runs before `deviceSecureProbe()`. Android can retain the private-key handle and certificate after an authentication-required key has been permanently invalidated by credential or lock-screen changes; this class's decrypt path explicitly handles that retained-handle behavior. Because encryption uses only the retained public key, new private scalars can be encrypted successfully under an alias whose private half will always throw `KeyPermanentlyInvalidatedException`. This also bypasses `requireAuthGated`: a strict manager on a now-lockless device reuses the alias instead of throwing, while `effectiveKeySecurityPolicy()` reports `AUTH_GATED` for the same unusable state. Probe the current lock-screen state before accepting an existing auth-gated alias, and apply the same ordering to effective-policy reporting.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:2831-2832: Pending-key reconstruction swallows coroutine cancellation
`runCatching` catches `CancellationException` thrown by the suspend `isPrivateKeyDecryptable` callback and converts it to `false`. Reconstruction can then mark the row pending and return normally, preventing callers such as `loadPersistedWallets()` and the invalidation-bookkeeping path from observing cancellation at this boundary. Preserve structured concurrency by rethrowing cancellation and converting only genuine probe failures to an unusable result.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1221-1229: Deleted identity keys remain in pendingIdentityKeys
`onPersistIdentityKeyRemoval` deletes the Room row without removing its entry from `_pendingIdentityKeys`. Identity removal and `deleteWalletDataLocked` have the same omission: Room cascades remove durable rows, but they cannot mutate the process-local `StateFlow`. After a failed derivation followed by key, identity, or wallet deletion, hosts continue receiving a repairable-key signal for a row that no longer exists, and repair fails because no derivation breadcrumbs remain. Stage key and identity pruning with the same changeset transaction so rollback preserves valid signals, and prune wallet-scoped entries only after the wallet-delete transaction succeeds.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:369-373: Generic signer messages can be reclassified as typed code 31
Both this catch-all and the direct `From<ProtocolError>` conversion at lines 467-472 search a fully rendered error with `contains(DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX)`. A foreign signer can return generic error code 0 with that token anywhere in its human-readable message, after which the platform-wallet ABI reports `ErrorSigningKeyUnavailable` (31) and hosts route an operational failure into key repair. Inspect the nested `ProtocolError::Generic` payload structurally and require the reserved marker at the payload start. The signer completion boundary should also escape or otherwise disambiguate generic-code messages that begin with the reserved marker, so only a typed `SigningKeyUnavailable` completion can create that representation.
Comment on lines
+166
to
+169
| private fun resolveIdentityKeysWriteAlias(): String { | ||
| if (keySecurityPolicy == KeySecurityPolicy.DEVICE_BOUND) return KEYS_ALIAS_DEVICE_BOUND | ||
| if (hasIdentityKeysKey(KEYS_ALIAS_AUTH_GATED)) return KEYS_ALIAS_AUTH_GATED | ||
| if (!deviceSecureProbe()) { |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Lock-screen removal still reuses the auth-gated alias
The auth-gated alias presence check runs before deviceSecureProbe(). Android can retain the private-key handle and certificate after an authentication-required key has been permanently invalidated by credential or lock-screen changes; this class's decrypt path explicitly handles that retained-handle behavior. Because encryption uses only the retained public key, new private scalars can be encrypted successfully under an alias whose private half will always throw KeyPermanentlyInvalidatedException. This also bypasses requireAuthGated: a strict manager on a now-lockless device reuses the alias instead of throwing, while effectiveKeySecurityPolicy() reports AUTH_GATED for the same unusable state. Probe the current lock-screen state before accepting an existing auth-gated alias, and apply the same ordering to effective-policy reporting.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 0d28d5e — Lock-screen removal still reuses the auth-gated alias no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Comment on lines
+2831
to
+2832
| val usable = row.privateKeyKeychainIdentifier != null && | ||
| runCatching { isPrivateKeyDecryptable(pubkeyHex) }.getOrDefault(false) |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Pending-key reconstruction swallows coroutine cancellation
runCatching catches CancellationException thrown by the suspend isPrivateKeyDecryptable callback and converts it to false. Reconstruction can then mark the row pending and return normally, preventing callers such as loadPersistedWallets() and the invalidation-bookkeeping path from observing cancellation at this boundary. Preserve structured concurrency by rethrowing cancellation and converting only genuine probe failures to an unusable result.
Suggested change
| val usable = row.privateKeyKeychainIdentifier != null && | |
| runCatching { isPrivateKeyDecryptable(pubkeyHex) }.getOrDefault(false) | |
| val usable = row.privateKeyKeychainIdentifier != null && | |
| try { | |
| isPrivateKeyDecryptable(pubkeyHex) | |
| } catch (cancellation: kotlin.coroutines.cancellation.CancellationException) { | |
| throw cancellation | |
| } catch (_: Throwable) { | |
| false | |
| } |
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 0d28d5e — Pending-key reconstruction swallows coroutine cancellation no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Comment on lines
+369
to
+373
| _ if error | ||
| .to_string() | ||
| .contains(rs_sdk_ffi::DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX) => | ||
| { | ||
| PlatformWalletFFIResultCode::ErrorSigningKeyUnavailable |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Generic signer messages can be reclassified as typed code 31
Both this catch-all and the direct From<ProtocolError> conversion at lines 467-472 search a fully rendered error with contains(DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX). A foreign signer can return generic error code 0 with that token anywhere in its human-readable message, after which the platform-wallet ABI reports ErrorSigningKeyUnavailable (31) and hosts route an operational failure into key repair. Inspect the nested ProtocolError::Generic payload structurally and require the reserved marker at the payload start. The signer completion boundary should also escape or otherwise disambiguate generic-code messages that begin with the reserved marker, so only a typed SigningKeyUnavailable completion can create that representation.
source: ['codex']
Collaborator
|
Round-4 verification (two independent passes, reconciled): the substance of all three fixes is verified — breadcrumb-driven repair with byte-verify backed by genuine external test vectors (ISO RIPEMD-160 + the canonical sipa pubkey), code-4 removal, and durable-write-before-state-clear ordering. Remaining:
|
Address the actionable review blockers on dashpay#4183 (keystore rework). Four blockers fixed; three items deliberately left for human decision (see PR notes) because a correct fix is not a minimal, verifiable edit. thepastaclaw blockers: - KeystoreManager.resolveIdentityKeysWriteAlias / effectiveKeySecurityPolicy: probe the CURRENT lock-screen state BEFORE accepting an existing auth-gated alias. Removing the secure lock screen permanently invalidates the auth-gated key's private half while Android retains its public half, so reusing the alias would encrypt new scalars under a key that can never decrypt (and silently bypass requireAuthGated). Both the write path and effective-policy reporting now degrade to DEVICE_BOUND on a now-lockless device. Test updated to assert the corrected semantics. - PlatformWalletPersistenceHandler.reconstructPendingIdentityKeysFromPersistence: the suspend isPrivateKeyDecryptable probe was wrapped in runCatching, which swallowed CancellationException and returned normally. Rethrow cancellation; only genuine probe failures become an unusable result. - PlatformWalletPersistenceHandler.onPersistIdentityKeyRemoval: a removed identity key stayed in pendingIdentityKeys as a phantom repair target. Stage a pending-clear delta with the round (published on commit, discarded on rollback) keyed by (identityId, keyId). Adds commit + rollback tests. shumkov P1 (correctness): - KeystoreSigner: the suspend onSigningKeyInvalidated callback was wrapped in runCatching (swallowing cancellation one layer too low), and the outer catch(Exception) would re-swallow a rethrow. Rethrow CancellationException at both sites so cancellation propagates instead of being masked as a native completion. Validation: :sdk:compileDebugKotlin + :sdk:compileDebugUnitTestKotlin clean; full :sdk:testDebugUnitTest 265 tests, 0 failures. Not applied (need human decision, documented in the PR report): - EDDSA (type-4) key repair: the resolver derive is ECDSA-only, so the Ed25519 public-key comparison can never match; a correct fix needs an Ed25519 pubkey derivation the Kotlin comparison path does not have (FFI change + native rebuild, or an Ed25519 impl matching dalek's seed convention). - Numeric lock-screen classifier (10309): the reviewer's claim that getNumericErrorCode() cannot return 10309 contradicts the existing tests that assert exactly that; the correct Android mechanism (message parse / getInternalErrorCode / a different public constant) cannot be determined without the reviewer's exact comment, and a wrong guess risks a security downgrade misclassification. - error.rs generic-signer misclassification: a classifier-only starts_with change does not close the hole because signer.rs emits a foreign generic-code message verbatim; a correct fix also needs producer-side marker escaping at the completion boundary (coordinated multi-file change, beyond a clean suggestion). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@thepastaclaw addressed all three blockers in
Also fixed the Separately flagging a real correctness bug I found while in here (not one of your three): type-4 EDDSA identity-key repair can never succeed — The numeric lock-screen classifier (10309) I also looked at but left untouched: the existing |
thepastaclaw
suggested changes
Jul 24, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The latest patch fixes the lock-screen alias ordering and pending-key reconstruction cancellation, and key-specific removals now clear their pending state transactionally. Two blocking issues remain: identity and wallet deletion retain stale repair entries, and signer cancellation abandons the required native completion token; generic signer text can also still be misclassified as typed code 31.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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 `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:1039-1041: Identity and wallet deletion still leave pending repair entries
Key-specific removal now stages a matching pending-state clear, but `onPersistIdentityRemoval` still stages only the Room identity deletion. Room's cascade removes the public-key rows without changing the process-local `_pendingIdentityKeys` StateFlow. `deleteWalletDataLocked` has the same omission: it deletes the wallet's identities and commits the wallet transaction without pruning entries whose `walletIdHex` belongs to that wallet. After either successful deletion, hosts continue receiving repair targets whose rows and derivation breadcrumbs no longer exist, so repair fails until the handler is recreated. Stage an identity-scoped clear in the identity-removal changeset, and prune wallet-scoped entries only after the wallet deletion transaction commits so rollback preserves valid signals.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt:98-104: Signer cancellation abandons the native completion token
After `signAsync` accepts a completion token, this cancellation path exits without invoking `SignerNative.completeSign`, despite `NativeSignerBridge` requiring exactly one completion per token. The Kotlin `scope.launch` job is independent of the Rust request, so rethrowing cancellation does not cancel the native receiver; Rust instead waits for the five-minute `SIGN_ASYNC_COMPLETION_TIMEOUT`, while the JNI `PendingSign` token remains allocated. The invalidation-bookkeeping callback can also route cancellation here before the already-known key-unavailable result is completed. Complete the token exactly once before unwinding—using the typed key-unavailable result when a `KeyPermanentlyInvalidatedException` is already known and a generic cancellation result elsewhere—or add an explicit native cancellation mechanism.
Comment on lines
+98
to
+104
| } catch (cancellation: kotlin.coroutines.cancellation.CancellationException) { | ||
| // NEVER swallow structured-concurrency cancellation (a teardown | ||
| // of the signer's IO scope): rethrow so the coroutine unwinds | ||
| // instead of masking the cancellation as a native completion | ||
| // (dashpay/platform#4183 review). The generic Exception handler | ||
| // below would otherwise catch it and call completeSign. | ||
| throw cancellation |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Signer cancellation abandons the native completion token
After signAsync accepts a completion token, this cancellation path exits without invoking SignerNative.completeSign, despite NativeSignerBridge requiring exactly one completion per token. The Kotlin scope.launch job is independent of the Rust request, so rethrowing cancellation does not cancel the native receiver; Rust instead waits for the five-minute SIGN_ASYNC_COMPLETION_TIMEOUT, while the JNI PendingSign token remains allocated. The invalidation-bookkeeping callback can also route cancellation here before the already-known key-unavailable result is completed. Complete the token exactly once before unwinding—using the typed key-unavailable result when a KeyPermanentlyInvalidatedException is already known and a generic cancellation result elsewhere—or add an explicit native cancellation mechanism.
source: ['codex']
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
3 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.
Stacked on #4191 (
port/v4.1/unmanaged-identity-reads) — review only thecommits above
ce3a17d71f. Once #4191 merges,git rebase --onto origin/v4.1-dev ce3a17d71fand force-push with--force-with-lease.This replaces the old single-squash
port/v4.1/android-integration(preserved at
backup/port-4060-pre-rework). That squash predated #4172 andsilently rewrote the exact regions #4172 later hardened; this branch is a
rebuild on the current base with each review finding resolved explicitly.
The unmanaged-identity reads and the text-classified SigningKeyUnavailable
from the old squash shipped separately as #4191 and are NOT duplicated here.
Scope — Keystore only, one logical commit per concern:
KeySecurityPolicy(AUTH_GATED default /DEVICE_BOUND opt-in), two dedicated RSA aliases, legacy
KEYS_ALIASread-only; fix(kotlin-sdk): harden identity-key Keystore recovery and reconcile parity docs #4172's fingerprint capture + invalidation recovery survive
alias-parameterized (finding 1).
redirect to the DEVICE_BOUND alias (never a silently gate-less key under
the auth-gated alias);
effectiveKeySecurityPolicy()+requireAuthGatedstrict mode; per-blob alias tag routes reads;tightened KeyMint failure classifier (finding 4).
never applies to legacy blobs, finding 2), fingerprint fast path with
KPIE rethrow-after-generation-checked-cleanup (brick-loop fix, finding 1),
mismatch/missing fp routes into former-RSA recovery, migration forward.
Cheap
canSignWithnever decrypts; the real-decrypt probe moved toprobeIdentityKeyRecoverability(finding 3).pendingIdentityKeysStateFlow,round-transactional staging.
restart reconstruction incl. blobs stranded by keypair replacement
(finding 5).
replacePrivateKey+forcederive path +real-decrypt verification before
markIdentityKeyRepaired(finding 6).PlatformWallet.NotFound(Swift parity; releasenote in the commit).
error_codeon the signcompletion ABI,
ErrorSigningKeyUnavailable = 31(26-30 reserved forfeat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4185/fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) #4184), Kotlin/Swift typed mappings; MESSAGE_MARKER sniff retained as a
deprecated fallback for the fix(kotlin-sdk): unmanaged-identity reads return absence + typed SigningKeyUnavailable (split from #4183) #4191 merge-order transition (mixed
old-native/new-Kotlin artifacts are unsupported — the completion JNI
arity changed) (finding 7). Known residual: the
Rust-internal segment rides a stable machine prefix through
ProtocolError::Generic— a typed rs-dpp variant was deliberatelyavoided (serialization blast radius).
InvalidKeyException failures were the pre-fix(kotlin-sdk): harden identity-key Keystore recovery and reconcile parity docs #4172 emulator-relock flake —
inherited workflow guard covers them).
convergent, pending-repair gap recorded for Swift, code-31 capability),
migration ladder v8 note.
CI expectations: emulator job runs
:sdk:connectedDebugAndroidTest(watchthe
deviceLocked=0guard); swift job rebuilds the xcframework so the5-arg completion ABI and code 31 compile against fresh headers.
Round-3 addendum: verification passes found and fixed three repair-routing defects (health-probe fingerprint disproof for replaced aliases, typed first-attempt invalidation errors, durable repair seeding for legacy and former-RSA keys — commits
6d1fa5be83,f6fe865e56,7ee6e4b7f9,ce3490a3a0). Unit suite: 249 passing; instrumented suite runs on this repo's emulator job.🤖 Generated with Claude Code
Summary by CodeRabbit