Skip to content

fix: refresh paykit key after wallet reset - #761

Open
ben-kaufman wants to merge 2 commits into
masterfrom
codex/reset-paykit-noise-key
Open

ben-kaufman wants to merge 2 commits into
masterfrom
codex/reset-paykit-noise-key

Conversation

@ben-kaufman

Copy link
Copy Markdown
Contributor

Description

This PR prevents private payment connections from reusing a previous wallet's Noise key after a wallet reset.

  • Checks that the cached key still matches the keychain before reusing it. When a reset removes the stored key, the existing store derives and persists the replacement wallet's key without requiring an app restart.
  • Keeps the existing key-length, wallet-seed, and unexpected-replacement validation. Repeated reads still reuse the validated derivation when the stored key is unchanged.
  • Adds two regression tests covering wallet replacement/restart consistency and changed or malformed persisted keys after the cache is populated.

No SDK update, key format change, migration, UI change, or peer-state reset is included. This fixes the reproduced local key-cache bug; it does not establish that the staging server retained an old peer key or repair any already-stalled remote handshake.

Linked Issues/Tasks

  • Android already contains the corresponding wallet-reset fix and regression coverage, merged in #1106, commit 64ca85f.

Design

N/A — no UI changes.

Screenshot / Video

N/A — no UI changes.

QA Notes

Manual Tests

  • 1. Disposable wallet with Paykit configured → Wipe Wallet → create a different wallet without terminating the app → configure Paykit: the new wallet publishes its own receiver key and can establish a private connection.
  • 2. Restart the replacement wallet: its receiver key stays the same and the private connection remains usable.

Automated Checks

  • BitkitTests/PaykitReceiverNoiseKeyStoreTests.swift: both new tests failed against the original code and passed with this fix; all seven key-store tests pass.
  • The normal CI unit-test selection passed 1,295 tests on a separate simulator, using the same live-integration exclusions as CI.
  • Android's existing PaykitSdkServiceTest passed all 15 tests on current master, including the wallet-replacement regression. Published dependencies were used instead of incompatible local Maven artifacts.
  • SwiftFormat and whitespace checks passed. The demo devices and existing PR worktrees were left untouched.

@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with the cache invalidation behavior covered by focused regression tests and no actionable correctness or security issue identified.

Summary

This PR makes the Paykit receiver Noise-key cache sensitive to changes in its keychain backing value, allowing an in-process wallet replacement to derive and persist the correct key.

  • Reloads persisted key bytes before reusing the validated in-memory derivation.
  • Invalidates the cache when the persisted key is removed or changed while preserving length and seed-consistency validation.
  • Adds regression coverage for wallet replacement, restart consistency, and changed or malformed persisted keys.
  • Adds the corresponding fixed changelog entry.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Request receiver Noise key] --> B[Load persisted key bytes]
    B --> C{Persisted key equals cached validated key?}
    C -- Yes --> D[Reuse cached key]
    C -- No --> E[Clear cached validation]
    E --> F[Derive key from current wallet seed]
    F --> G{Persisted key exists?}
    G -- No --> H[Persist derived replacement key]
    G -- Yes --> I{Persisted key is valid and matches derivation?}
    I -- Yes --> J[Cache and return validated key]
    I -- No --> K[Reject inconsistent key material]
    H --> J
Loading

Reviews (1) · Last reviewed commit: "fix: refresh paykit key after wallet res..."

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings.

Checked:

  • No stale key after a reset. Every wallet replacement goes through AppReset.wipeKeychain.wipeEntireKeychain(), which deletes paykit_receiver_noise_secret_key along with the mnemonic. loadBytes() then returns nil, the cache clears, and the new seed's key is derived and upserted.
  • Locked-device read. Keychain.load throws for any status other than errSecItemNotFound, so an errSecInteractionNotAllowed read fails before the cache is cleared or anything is written. A mismatched stored key is rejected, never overwritten.
  • Concurrency. loadOrDerive and persist hold the store's NSLock across the whole check-then-set. The injected closures don't re-enter the store, and lock order is always provider → store. A wipe racing a sign-in fails at persist instead of persisting a stale key.
  • Upgrade from v2.5.0 (pre-fix store). When stored matches derived, the first call re-validates and caches. No migration is needed.
  • Tests. Both new tests fail on the pre-fix code, where the cached bytes were returned.
  • Parity with synonymdev/bitkit-android@64ca85f2c. Android dropped the cache; iOS keys the cache on keychain equality. The outcome is the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants