Merge bitcoin/bitcoin#28724: wallet: Cleanup accidental encryption keys in watchonly wallets

69e95c2b4f tests: Test cleanup of mkeys from wallets without privkeys (Andrew Chow)
2b9279b50a wallet: Remove unused encryption keys from watchonly wallets (Andrew Chow)
813a16a463 wallet: Add HasCryptedKeys (Andrew Chow)

Pull request description:

  An earlier version allowed users to create watchonly wallets (wallets without private keys) that were "encrypted". Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as https://github.com/bitcoin-core/gui/issues/772.

  We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those encryption keys thereby avoiding any issues that may result from this unexpected situation.

ACKs for top commit:
  sipa:
    utACK 69e95c2b4f.
  laanwj:
    Code review re-ACK 69e95c2b4f
  furszy:
    Code review ACK 69e95c2b4f

Tree-SHA512: 901932cd709c57e66c598f011f0105a243b5a8b539db2ef3fcf370dca4cf35ae09bc1110e8fca8353be470f159468855a4dd96b99bc9c1112adc86ccc50e1b9d

src/wallet/scriptpubkeyman.cpp

@@ -525,6 +525,12 @@ bool LegacyScriptPubKeyMan::HavePrivateKeys() const
     return !mapKeys.empty() || !mapCryptedKeys.empty();
 }
 
+bool LegacyScriptPubKeyMan::HaveCryptedKeys() const
+{
+    LOCK(cs_KeyStore);
+    return !mapCryptedKeys.empty();
+}
+
 void LegacyScriptPubKeyMan::RewriteDB()
 {
     LOCK(cs_KeyStore);
@@ -2411,6 +2417,12 @@ bool DescriptorScriptPubKeyMan::HavePrivateKeys() const
     return m_map_keys.size() > 0 || m_map_crypted_keys.size() > 0;
 }
 
+bool DescriptorScriptPubKeyMan::HaveCryptedKeys() const
+{
+    LOCK(cs_desc_man);
+    return !m_map_crypted_keys.empty();
+}
+
 std::optional<int64_t> DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const
 {
     // This is only used for getwalletinfo output and isn't relevant to descriptor wallets.

src/wallet/scriptpubkeyman.h

@@ -221,6 +221,7 @@ public:
     virtual bool Upgrade(int prev_version, int new_version, bilingual_str& error) { return true; }
 
     virtual bool HavePrivateKeys() const { return false; }
+    virtual bool HaveCryptedKeys() const { return false; }
 
     //! The action to do when the DB needs rewrite
     virtual void RewriteDB() {}
@@ -473,6 +474,7 @@ public:
     bool Upgrade(int prev_version, int new_version, bilingual_str& error) override;
 
     bool HavePrivateKeys() const override;
+    bool HaveCryptedKeys() const override;
 
     void RewriteDB() override;
 
@@ -661,6 +663,7 @@ public:
     bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
     //! Retrieve the particular key if it is available. Returns nullopt if the key is not in the wallet, or if the wallet is locked.
     std::optional<CKey> GetKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
+    bool HaveCryptedKeys() const override;
 
     std::optional<int64_t> GetOldestKeyPoolTime() const override;
     unsigned int GetKeyPoolSize() const override;

src/wallet/wallet.cpp

@@ -3712,6 +3712,14 @@ bool CWallet::HasEncryptionKeys() const
     return !mapMasterKeys.empty();
 }
 
+bool CWallet::HaveCryptedKeys() const
+{
+    for (const auto& spkm : GetAllScriptPubKeyMans()) {
+        if (spkm->HaveCryptedKeys()) return true;
+    }
+    return false;
+}
+
 void CWallet::ConnectScriptPubKeyManNotifiers()
 {
     for (const auto& spk_man : GetActiveScriptPubKeyMans()) {

src/wallet/wallet.h

@@ -973,6 +973,7 @@ public:
     bool WithEncryptionKey(std::function<bool (const CKeyingMaterial&)> cb) const override;
 
     bool HasEncryptionKeys() const override;
+    bool HaveCryptedKeys() const;
 
     /** Get last block processed height */
     int GetLastBlockHeight() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)

src/wallet/walletdb.cpp

@@ -154,6 +154,11 @@ bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)
     return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true);
 }
 
+bool WalletBatch::EraseMasterKey(unsigned int id)
+{
+    return EraseIC(std::make_pair(DBKeys::MASTER_KEY, id));
+}
+
 bool WalletBatch::WriteCScript(const uint160& hash, const CScript& redeemScript)
 {
     return WriteIC(std::make_pair(DBKeys::CSCRIPT, hash), redeemScript, false);
@@ -1241,6 +1246,21 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
         result = DBErrors::CORRUPT;
     }
 
+    // Since it was accidentally possible to "encrypt" a wallet with private keys disabled, we should check if this is
+    // such a wallet and remove the encryption key records to avoid any future issues.
+    // Although wallets without private keys should not have *ckey records, we should double check that.
+    // Removing the mkey records is only safe if there are no *ckey records.
+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) {
+        pwallet->WalletLogPrintf("Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys.\n");
+        for (const auto& [id, _] : pwallet->mapMasterKeys) {
+            if (!EraseMasterKey(id)) {
+                pwallet->WalletLogPrintf("Error: Unable to remove extraneous encryption key '%u'. Wallet corrupt.\n", id);
+                return DBErrors::CORRUPT;
+            }
+        }
+        pwallet->mapMasterKeys.clear();
+    }
+
     return result;
 }
 

src/wallet/walletdb.h

@@ -243,6 +243,7 @@ public:
     bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata &keyMeta);
     bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector<unsigned char>& vchCryptedSecret, const CKeyMetadata &keyMeta);
     bool WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey);
+    bool EraseMasterKey(unsigned int id);
 
     bool WriteCScript(const uint160& hash, const CScript& redeemScript);
 

test/functional/wallet_encryption.py

@@ -5,7 +5,9 @@
 """Test Wallet encryption"""
 
 import time
+import subprocess
 
+from test_framework.messages import hash256
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_raises_rpc_error,
@@ -100,6 +102,65 @@ class WalletEncryptionTest(BitcoinTestFramework):
             sig = self.nodes[0].signmessage(address, msg)
             assert self.nodes[0].verifymessage(address, sig, msg)
 
+        self.log.info("Test that wallets without private keys cannot be encrypted")
+        self.nodes[0].createwallet(wallet_name="noprivs", disable_private_keys=True)
+        noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs")
+        assert_raises_rpc_error(-16, "Error: wallet does not contain private keys, nothing to encrypt.", noprivs_wallet.encryptwallet, "pass")
+
+        if self.is_wallet_tool_compiled():
+            self.log.info("Test that encryption keys in wallets without privkeys are removed")
+
+            def do_wallet_tool(*args):
+                proc = subprocess.Popen(
+                    [self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}"] + list(args),
+                    stdin=subprocess.PIPE,
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE,
+                    text=True
+                )
+                stdout, stderr = proc.communicate()
+                assert_equal(proc.poll(), 0)
+                assert_equal(stderr, "")
+
+            # Since it is no longer possible to encrypt a wallet without privkeys, we need to force one into the wallet
+            # 1. Make a dump of the wallet
+            # 2. Add mkey record to the dump
+            # 3. Create a new wallet from the dump
+
+            # Make the dump
+            noprivs_wallet.unloadwallet()
+            dumpfile_path = self.nodes[0].datadir_path / "noprivs.dump"
+            do_wallet_tool("-wallet=noprivs", f"-dumpfile={dumpfile_path}", "dump")
+
+            # Modify the dump
+            with open(dumpfile_path, "r", encoding="utf-8") as f:
+                dump_content = f.readlines()
+            # Drop the checksum line
+            dump_content = dump_content[:-1]
+            # Insert a valid mkey line. This corresponds to a passphrase of "pass".
+            dump_content.append("046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n")
+            with open(dumpfile_path, "w", encoding="utf-8") as f:
+                contents = "".join(dump_content)
+                f.write(contents)
+                checksum = hash256(contents.encode())
+                f.write(f"checksum,{checksum.hex()}\n")
+
+            # Load the dump into a new wallet
+            do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "createfromdump")
+            # Load the wallet and make sure it is no longer encrypted
+            with self.nodes[0].assert_debug_log(["Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys."]):
+                self.nodes[0].loadwallet("noprivs_enc")
+            noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs_enc")
+            assert_raises_rpc_error(-15, "Error: running with an unencrypted wallet, but walletpassphrase was called.", noprivs_wallet.walletpassphrase, "pass", 1)
+            noprivs_wallet.unloadwallet()
+
+            # Make a new dump and check that there are no mkeys
+            dumpfile_path = self.nodes[0].datadir_path / "noprivs_enc.dump"
+            do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "dump")
+            with open(dumpfile_path, "r", encoding="utf-8") as f:
+                # Check theres nothing with an 'mkey' prefix
+                assert_equal(all([not line.startswith("046d6b6579") for line in f]), True)
+
 
 if __name__ == '__main__':
     WalletEncryptionTest(__file__).main()