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. 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. These paths are not remotely triggerable.

Fix: For wallet encryption, publish live key state only after all required database writes and erases succeed and the transaction commits. For passphrase changes, encrypt a copy of the master key and replace the live master key only after the database write succeeds. The covered failures leave database and memory unchanged so wallet encryption and passphrase changes can be retried. Five fault-injection tests exercise these paths through the public wallet interface.

Follow-up to #35500

<!--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:

• #35852 (scripted-diff: Use inline const(expr) over static constexpr in headers by maflcko)
• #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
• #34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
• #34400 (wallet: parallel fast rescan (approx 8x speed up with 8 threads) by Eunovo)
• #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
• #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-->

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>