Merge 35bf426e02 into merged_master (Bitcoin PR bitcoin/bitcoin#28724)

src/wallet/scriptpubkeyman.cpp

@@ -531,6 +531,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);
@@ -2416,6 +2422,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;
 
@@ -664,6 +666,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

@@ -4284,6 +4284,14 @@ void CWalletTx::GetNonIssuanceBlindingData(const CWallet& wallet, const unsigned
         blinding_pubkey_out, value_out, value_factor_out, asset_out, asset_factor_out);
 }
 
+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

@@ -1047,6 +1047,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);
@@ -1384,6 +1389,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()