wallet: make encryption state updates atomic #35752

Problem: Wallet encryption and passphrase changes update database records and live key state, but local database failures can leave them out of sync. Wallet encryption can report success after a master-key write fails, while a passphrase change can update only memory. A re-encryption failure can also leave a previously locked wallet unlocked. Descriptor key write failures can publish keys that were not persisted, and erase failures can commit both plaintext and encrypted records. A failed transaction commit instead aborts the node after publishing live encryption state. Wallet unlock and passphrase changes return only a boolean, forcing callers to duplicate or conflate error handling. The database failures cannot be triggered remotely.

Fix: For wallet encryption, publish the live master key and encrypted state of existing descriptors only after their database writes and erases succeed and the transaction commits. For passphrase changes, restore the original lock state after validating the old passphrase, encrypt a copy of the master key, and replace the live master key only after the database write succeeds. Return structured errors from wallet unlock and passphrase changes so callers can reuse messages and distinguish incorrect passphrases from encryption and database failures. Publish newly inserted descriptor keys only after their database records are written. The covered failures leave database and live key state unchanged, allowing each operation to be retried. Fresh descriptor setup after the encryption transaction remains unchanged. Fault-injection tests exercise these paths through the public wallet interface.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35752.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36087 (util: Add and use AssertUnreachable by maflcko)
• #36074 (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)
• #36070 (wallet: Add deriveHDKey interface by PraneethGunas)
• #36033 ([wip,nomerge,rfc] build: Require C++23 compiler by maflcko)
• #36031 (wallet: Remove mapMasterKeys and enforce that only one encryption key can exist by achow101)
• #35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
• #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
• #34861 (wallet: Add importdescriptors interface by polespinasa)
• #34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
• #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
• #29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

• assert_raises_rpc_error(-14, ..., self.nodes[0].walletpassphrase, passphrase_with_nulls + "\0", 10) in test/functional/wallet_encryption.py

Possible places where comparison-specific test macros should replace generic comparisons:

• [src/wallet/test/wallet_tests.cpp] BOOST_CHECK_THROW((void)add_key(), std::runtime_error); -> use BOOST_CHECK_EXCEPTION(..., std::runtime_error, <reason matcher>) so the test checks the expected failure message instead of only the generic exception type.

^{2026-08-20 03:07:06}

Updated the failure handling and tests after review. Descriptor encryption now stages in-memory key updates until the database transaction commits, propagates encrypted-key write and plaintext-key erase failures, and returns false instead of aborting so encryption can be retried. The fault-injection tests exercise these failures through the public wallet interface. Turning to draft to get more conceptual review.

Concept ACK

Concept ACK

I found the last commit particularly difficult to review because it changes WriteCryptedDescriptorKey() from ignoring the result of EraseIC(), as master does, to propagating it.

I'm not sure it's handling failure propagation safely when AddDescriptorKeyWithDB() adds a private key to an already encrypted wallet: the encrypted key may already have been committed, leaving a partial state while still reporting failure.

Something like the suggestion below may avoid this case, though there may be a better approach.

The failure case is very narrow, but it may still be worth addressing.

<details> <summary>suggestion</summary>

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index dea90f70eb..b509327f13 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -966,7 +966,7 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle
         if (!EncryptSecret(master_key, secret, pubkey.GetHash(), crypted_secret)) {
             return false;
         }
-        if (!batch->WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret)) {
+        if (!batch->WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret, /*erase_plaintext_key=*/true)) {
             return false;
         }
         crypted_keys[pubkey.GetID()] = make_pair(pubkey, std::move(crypted_secret));
@@ -1185,7 +1185,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
         }
 
         m_map_crypted_keys[pubkey.GetID()] = make_pair(pubkey, crypted_secret);
-        return batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret);
+        return batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret, /*erase_plaintext_key=*/false);
     } else {
         m_map_keys[pubkey.GetID()] = key;
         return batch.WriteDescriptorKey(GetID(), pubkey, key.GetPrivKey());
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 9f9c4e634e..06f49fd5e1 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -156,9 +156,20 @@ struct EncryptionFailureSetup : WalletTestingSetup {
     {
         context.args = &m_args;
         context.chain = m_node.chain.get();
+        CreateWallet(WALLET_FLAG_DESCRIPTORS);
+    }
+
+    void CreateWallet(uint64_t create_flags)
+    {
         auto database{std::make_unique<FaultInjectingDatabase>()};
         fail_db = database.get();
-        wallet = TestCreateWallet(std::move(database), context, WALLET_FLAG_DESCRIPTORS);
+        wallet = TestCreateWallet(std::move(database), context, create_flags);
+    }
+
+    void RecreateWallet(uint64_t create_flags)
+    {
+        TestUnloadWallet(std::move(wallet));
+        CreateWallet(create_flags);
     }
 
     ~EncryptionFailureSetup() { TestUnloadWallet(std::move(wallet)); }
@@ -231,6 +242,27 @@ BOOST_FIXTURE_TEST_CASE(encrypt_wallet_descriptor_key_erase_failure, EncryptionF
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(add_encrypted_descriptor_key_skips_plaintext_erase, EncryptionFailureSetup)
+{
+    BOOST_REQUIRE(wallet->EncryptWallet("passphrase"));
+    BOOST_REQUIRE(wallet->Unlock("passphrase"));
+
+    fail_db->FailNextErase(DBKeys::WALLETDESCRIPTORKEY);
+    AddKey(*wallet, GenerateRandomKey());
+}
+
+BOOST_FIXTURE_TEST_CASE(encrypt_wallet_fresh_descriptor_keys_skip_plaintext_erase, EncryptionFailureSetup)
+{
+    // Match the pre-encryption state of a non-blank wallet created with a passphrase: no plaintext keys or descriptors.
+    RecreateWallet(WALLET_FLAG_DESCRIPTORS | WALLET_FLAG_BLANK_WALLET);
+    wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
+
+    fail_db->FailNextErase(DBKeys::WALLETDESCRIPTORKEY);
+    BOOST_REQUIRE(wallet->EncryptWallet("passphrase"));
+    BOOST_CHECK(wallet->HasEncryptionKeys());
+    BOOST_CHECK(wallet->IsLocked());
+}
+
 BOOST_FIXTURE_TEST_CASE(update_non_range_descriptor, TestingSetup)
 {
     CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 0916dcaa47..cc8ce80399 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -222,12 +222,13 @@ bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubk
     return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)), std::make_pair(privkey, keypair_hash), false);
 }
 
-bool WalletBatch::WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret)
+bool WalletBatch::WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey,
+                                            const std::vector<unsigned char>& secret, bool erase_plaintext_key)
 {
     if (!WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORCKEY, std::make_pair(desc_id, pubkey)), secret, false)) {
         return false;
     }
-    return EraseIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)));
+    return !erase_plaintext_key || EraseIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)));
 }
 
 bool WalletBatch::WriteDescriptor(const uint256& desc_id, const WalletDescriptor& descriptor)
diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h
index 8397fff9cc..c75261557d 100644
--- a/src/wallet/walletdb.h
+++ b/src/wallet/walletdb.h
@@ -249,7 +249,9 @@ public:
     bool WriteOrderPosNext(int64_t nOrderPosNext);
 
     bool WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey);
-    bool WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret);
+    //! Erase the plaintext key only when converting an existing key, not when adding a new encrypted key.
+    bool WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey,
+                                   const std::vector<unsigned char>& secret, bool erase_plaintext_key);
     bool WriteDescriptor(const uint256& desc_id, const WalletDescriptor& descriptor);
     bool WriteDescriptorDerivedCache(const CExtPubKey& xpub, const uint256& desc_id, uint32_t key_exp_index, uint32_t der_index);
     bool WriteDescriptorParentCache(const CExtPubKey& xpub, const uint256& desc_id, uint32_t key_exp_index);

</details>

Moved over to #35998, thanks @achow101.

Moved over to #35998, thanks @achow101.

bruh

I didn't open that pr in order to take this one over.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/32073588819/job/95521863865} _{LLM reason (✨ experimental): clang-tidy failed (warnings-as-errors) due to bugprone-unused-return-value in /src/wallet/test/wallet_tests.cpp (BOOST_CHECK_THROW/add_key() return value ignored).}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

I don't like the last commit, I don't think all of that refactoring and additional code in areas that shouldn't need to care about underlying encryption is worth the benefit that it gives.

Ultimately, what is the point of the last commit? It seems like it's trying to make the entirety of EncryptWallet atomic, so if anything fails in setting up the new descriptors, the wallet is reverted to be unencrypted and still usable to the user? I'm not sure that having all of this code to handle an error here is meaningfully useful, given that it is a corruption scenario; the normal paths become unnecessarily complicated.

In many corruption scenarios, I think it is ok/better for us to unload the wallet and tell the user corruption occurred and that they should restore a backup. We do not need to try so hard to recover from corruption scenarios; restoring previous state and staying operational is not a necessity.

The easiest way to ensure in-memory state matches disk is also to just unload and reload, rather than doing a bunch of caching/staging or whatever.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/32092273017/job/95576690175} _{LLM reason (✨ experimental): CI failed because clang-tidy reported an error (bugprone-argument-comment) where the overwrite comment name didn’t match the fOverwrite parameter name in walletdb.cpp.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

It took me a few rounds to get to a version that is simple enough, and the previous pushes contained needless refactors. Thanks for the reviews, undrafting, ready for review again!

AI review revealed that passphrase errors could be confused with master-key write failures, so I extended the error messages and tests to cover both cases. Also added database checks for master and descriptor key records that were missed agyer previous review.

https://github.com/achow101/bitcoin/commit/99963d18c99da443fcfcd768faaca848237e2c91 for my suggestions

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/32326323514/job/96298202338} _{LLM reason (✨ experimental): CI failed due to a lint violation: a fatal assert(false) was used in RPC code (src/wallet/rpc/encrypt.cpp), triggering the rpc_assert check.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

ACK 50f07b23282336629352772e72847364e61d0c94

tACK 50f07b23282336629352772e72847364e61d0c94

Built and ran the tests. Left two comments inline.

ACK 50f07b23282336629352772e72847364e61d0c94