[kiwidream]

Summary

This closes three races around CWallet::EncryptWallet from #179. They predate #178, but #178's asynchronous GUI encryption makes it easier for a GUI encryption and an encryptwallet RPC to overlap.

1. Check-then-encrypt was not atomic. The already-encrypted and PQC-validation checks ran before LOCK2. An encryptwallet RPC overlapping the GUI worker could pass them too, double-encrypt and hit assert(false).
   • EncryptWallet now makes both checks under LOCK2(m_relock_mutex, cs_wallet), after the key derivation, which still runs unlocked on the GUI and interfaces path.
   • The RPC takes its locks before its own check, so an overlapping or repeated call now fails deterministically with -15 RPC_WALLET_WRONG_ENC_STATE ("running with an encrypted wallet") instead of -16 "Failed to encrypt". -16 now means a real database failure. This is a user-visible RPC error-code change.
2. A master key leaked into memory on TxnBegin failure. The key now enters mapMasterKeys and nMasterKeyMaxID only after TxnBegin and WriteMasterKey both succeed. A failed WriteMasterKey now aborts the transaction. Post-commit behaviour is unchanged.
3. Unlocked readers of the SPKM maps.
   • GetActiveScriptPubKeyMans, IsActiveScriptPubKeyMan and ConnectScriptPubKeyManNotifiers now require cs_wallet, via EXCLUSIVE_LOCKS_REQUIRED plus AssertLockHeld.
   • IsHDEnabled now locks internally.
   • The two unlocked callers, AddWallet and CWallet::Create, are fixed.
   • The only GUI caller of hdEnabled is already skipped while encrypting (qt: keep Encrypt Wallet responsive during existing-wallet encryption #178).
4. GUI follow-up. If another request wins the race, the Encrypt Wallet dialog now says "The wallet was already encrypted by another request. Your passphrase was not used." Before, it said the wallet was not encrypted. It reads the status after the worker has returned.

There are no wallet format, encryption semantics or consensus changes.

Fixes #179.

Testing

• Built locally.

• Ran focused unit or functional tests for the changed area.

• Ran lint or formatting checks relevant to this change.

• Not run. Reason:

• New wallet_encryption_tests suite, 3 deterministic cases using a test-only hook between key derivation and locking, plus the mock database:

  • two overlapping EncryptWallet calls;
  • TxnBegin failure;
  • WriteMasterKey failure.

  Against the base production code these fail with SIGABRT at assert(false), with IsCrypted/mapMasterKeys left set, and with a fatal flag-write error respectively.

• wallet_encryption.py adds a two-thread overlapping encryptwallet case. It got -16 on 6/6 base runs and gets -15 with the fix.

• New Qt test TestEncryptWalletAlreadyEncryptedByAnotherRequest covers the dialog message and fails without the fix. The race test now fails instead of hanging if its first assertion fails; verified with a forced failure.

• Other suites: test_qbit wallet suites, wallet_encryption.py (also with --usecli), wallet_keypool.py and test_qbit-qt (offscreen, including the qt: keep Encrypt Wallet responsive during existing-wallet encryption #178 tests) pass.

• clang 20 with -Wthread-safety -Wthread-safety-precise -Werror=thread-safety and DEBUG_LOCKORDER: all 65 wallet and wallet-test objects rebuild clean. Its wallet, spend, psbt and feebumper suites pass, and wallet_encryption_tests passes 15/15 repeats.

• Adversarial review: an independent review traced every caller of the changed accessors for lock-order inversions and GUI freezes, checked every EncryptWallet failure path and the RPC lock scope, and reproduced the revert proofs. It found no blocker; its nits are addressed in 91f83c5.

• Not run: TSan, Debug-config, macOS/Windows, the Qt translation units under clang thread-safety (the clang build is GUI-off), and a manual GUI+RPC overlap on a populated wallet. .ts translation files weren't regenerated for the new string.

Follow-up (not in this PR): GetAllScriptPubKeyMans, GetScriptPubKeyMan and IsCrypted remain unannotated unlocked readers.

Target Branch

• This PR targets main or a maintainer-requested release branch such as 0.1.x.

Targets 1.x.x for v1.0.1 (#174). It merges cleanly with #182.

Risk / Review Notes

• Consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.
• No consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.

Notes: this changes wallet encryption locking and failure handling, plus lock requirements on SPKM accessors. The RPC error code for an overlapping or repeated encryptwallet changes from -16 to -15, and the v1.0.1 release notes should mention it.

Docs / Process Impact

Choose exactly one:

• I updated public docs because this PR changes user-visible behavior, integration guidance, release/process guidance, or expected validation.
• No public docs update needed. Reason: the one user-visible change, the -15 code for an already-encrypted wallet, will be added to the v1.0.1 release notes in release: prepare v1.0.1 #181.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-25T20:39:26.043487Z	91f83c5	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.