Skip to content

fix: refresh paykit key after wallet reset - #761

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

ben-kaufman wants to merge 5 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.wipe → Keychain.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.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Advice: ✅ Approve


Review: diff 3 files.
Counterpart synonymdev/bitkit-android#1106: equivalent.

Findings:
1 inline (non-blocking)

Audit:
Awaits QA.

Coverage:
QA: journeys and manual tests await green CI checks


Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)

Comment thread Bitkit/Services/PubkyService.swift

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Advice: ✅ Approve


Reaudit: diff 1 file.
No new findings; the rest is in the review.
Counterpart synonymdev/bitkit-android#1106: equivalent.


Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)

@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.

Delta since 7ba939a8e: three master merges and 8b98438c, which adds testCachedKeyDoesNotBypassKeychainReadFailure. No production change since my earlier review. No findings.

Checked and clean at head:

  • The cache fix itself. validatedKeyBytes() reads the keychain first and reuses the cache only when the bytes still match, and it nils the cache before deriving, so a derive that fails between a wipe and a new wallet leaves nothing stale behind. The session provider lives on the shared actor, which is why the cache survived a reset in the first place.
  • Derivation is deterministic from the seed and passphrase, so a restore on any device reproduces the same key. ReceiverNoiseSecretKey.random() has no callers in the app.
  • No derive-before-seed. The mnemonic and passphrase are saved in one function, saveString refuses to overwrite, and every loadOrDerive caller is user-driven or needs a stored session that the wipe removes.
  • Wipe. The noise secret has exactly one storage key and wipeEntireKeychain() removes it along with the mnemonic, the passphrase and the SDK state. The SDK blob holds only the noise public key. forgetSessionAccess takes the operation lock, so an in-flight sign-in cannot land after the wipe.
  • Reinstall. A fresh install with no installation marker wipes the keychain before anything else, so an orphaned key cannot be inherited; and a leftover key with no mnemonic is rejected rather than used.
  • Counterparties fetch the receiver noise public key per operation rather than pinning it, so a reset does not misdirect a payment; a payer holding the old identity targets a dead identity.
  • Late auth completion. A reset clears the active request, so the continuation throws before activation, and persist re-validates against the keychain.
  • Parity. Android already shipped the equivalent in 64ca85f2c (inside synonymdev/bitkit-android#1106, in v2.5.0). It dropped the cache entirely; iOS keeps it but re-checks the bytes. Same outcome.

@ovitrif ovitrif added this to the 2.6.0 milestone Sep 24, 2026
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.

4 participants