Merge pull request #5531 from str4d/overhaul-legacy-transparent-keypool

Only use legacy transparent keypool for internal keys

qa/rpc-tests/keypool.py

@@ -40,13 +40,13 @@ class KeyPoolTest(BitcoinTestFramework):
         bitcoind_processes[0].wait()
         # Restart node 0
         nodes[0] = start_node(0, self.options.tmpdir)
-        # Keep creating keys
+        # We can't create any external addresses, which don't use the keypool.
-        addr = nodes[0].getnewaddress()
+        # We should get an error that we need to unlock the wallet.
         try:
             addr = nodes[0].getnewaddress()
-            raise AssertionError('Keypool should be exhausted after one address')
+            raise AssertionError('Wallet should be locked.')
         except JSONRPCException as e:
-            assert(e.error['code']==-12)
+            assert_equal(e.error['code'], -13)
 
         # put three new keys in the keypool
         nodes[0].walletpassphrase('test', 12000)

qa/rpc-tests/walletbackup.py

@@ -96,10 +96,10 @@ class WalletBackupTest(BitcoinTestFramework):
         self.sync_all()
 
     # As above, this mirrors the original bash test.
-    def start_three(self):
+    def start_three(self, extra_args=None):
-        self.nodes[0] = start_node(0, self.options.tmpdir)
+        self.nodes[0] = start_node(0, self.options.tmpdir, extra_args)
-        self.nodes[1] = start_node(1, self.options.tmpdir)
+        self.nodes[1] = start_node(1, self.options.tmpdir, extra_args)
-        self.nodes[2] = start_node(2, self.options.tmpdir)
+        self.nodes[2] = start_node(2, self.options.tmpdir, extra_args)
         connect_nodes(self.nodes[0], 3)
         connect_nodes(self.nodes[1], 3)
         connect_nodes(self.nodes[2], 3)
@@ -191,6 +191,29 @@ class WalletBackupTest(BitcoinTestFramework):
         self.start_three()
         sync_blocks(self.nodes)
 
+        # We made extra transactions that involved addresses generated after the
+        # backups were taken, and external addresses do not use the keypool, so
+        # the balances shouldn't line up.
+        balance0backup = self.nodes[0].getbalance()
+        balance1backup = self.nodes[1].getbalance()
+        balance2backup = self.nodes[2].getbalance()
+        assert(balance0backup != balance0)
+        assert(balance1backup != balance1)
+        assert(balance2backup != balance2)
+
+        # However, because addresses are derived deterministically, we can
+        # recover the balances by generating the extra addresses and then
+        # rescanning.
+        for i in range(5):
+            self.nodes[0].getnewaddress()
+            self.nodes[1].getnewaddress()
+            self.nodes[2].getnewaddress()
+
+        logging.info("Re-starting nodes with -rescan")
+        self.stop_three()
+        self.start_three(['-rescan'])
+        sync_blocks(self.nodes)
+
         assert_equal(self.nodes[0].getbalance(), balance0)
         assert_equal(self.nodes[1].getbalance(), balance1)
         assert_equal(self.nodes[2].getbalance(), balance2)
@@ -215,9 +238,9 @@ class WalletBackupTest(BitcoinTestFramework):
 
         sync_blocks(self.nodes)
 
-        assert_equal(self.nodes[0].getbalance(), balance0)
+        assert_equal(self.nodes[0].getbalance(), balance0backup)
-        assert_equal(self.nodes[1].getbalance(), balance1)
+        assert_equal(self.nodes[1].getbalance(), balance1backup)
-        assert_equal(self.nodes[2].getbalance(), balance2)
+        assert_equal(self.nodes[2].getbalance(), balance2backup)
 
 
 if __name__ == '__main__':

src/wallet/rpcwallet.cpp

@@ -182,14 +182,11 @@ UniValue getnewaddress(const UniValue& params, bool fHelp)
     const CChainParams& chainparams = Params();
     EnsureWalletIsBackedUp(chainparams);
 
-    if (!pwalletMain->IsLocked())
+    EnsureWalletIsUnlocked();
-        pwalletMain->TopUpKeyPool();
 
     // Generate a new key that is added to wallet
-    std::optional<CPubKey> newKey = pwalletMain->GetKeyFromPool();
+    CPubKey newKey = pwalletMain->GenerateNewKey(true);
-    if (!newKey.has_value())
+    CKeyID keyID = newKey.GetID();
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
-    CKeyID keyID = newKey.value().GetID();
 
     std::string dummy_account;
     pwalletMain->SetAddressBook(keyID, dummy_account, "receive");

src/wallet/wallet.cpp

@@ -271,12 +271,12 @@ CPubKey CWallet::GenerateNewKey(bool external)
     transparent::AccountKey accountKey = this->GetLegacyAccountKey();
     std::optional<CPubKey> pubkey = std::nullopt;
     do {
-        auto index = hdChain.GetLegacyTKeyCounter();
+        auto index = hdChain.GetLegacyTKeyCounter(external);
         auto key = external ?
             accountKey.DeriveExternalSpendingKey(index) :
             accountKey.DeriveInternalSpendingKey(index);
 
-        hdChain.IncrementLegacyTKeyCounter();
+        hdChain.IncrementLegacyTKeyCounter(external);
         if (key.has_value()) {
             pubkey = AddTransparentSecretKey(
                 hdChain.GetSeedFingerprint(),
@@ -2279,7 +2279,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 
         Lock();
         Unlock(strWalletPassphrase);
-        NewKeyPool();
+        // TODO: migrate to a new mnemonic when encrypting an unencrypted wallet?
         Lock();
 
         // Need to completely rewrite the wallet file; if we don't, bdb might keep
@@ -3122,6 +3122,11 @@ void CWallet::GenerateNewSeed(Language language)
     // as a hdchain object
     CHDChain newHdChain(seed.Fingerprint(), nCreationTime);
     SetMnemonicHDChain(newHdChain, false);
+
+    // Now that we can derive keys deterministically, clear out the legacy
+    // transparent keypool of all randomly-generated keys, and fill it with
+    // internal keys (for use as change addresses or miner outputs).
+    NewKeyPool();
 }
 
 bool CWallet::SetMnemonicSeed(const MnemonicSeed& seed)
@@ -5024,8 +5029,9 @@ bool CWallet::SetDefaultKey(const CPubKey &vchPubKey)
 }
 
 /**
- * Mark old keypool keys as used,
+ * Mark old keypool keys as used, and derive new internal keys.
- * and generate all new keys
+ *
+ * This is only used when first migrating to HD-derived transparent keys.
  */
 bool CWallet::NewKeyPool()
 {
@@ -5043,7 +5049,7 @@ bool CWallet::NewKeyPool()
         for (int i = 0; i < nKeys; i++)
         {
             int64_t nIndex = i+1;
-            walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey(true)));
+            walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey(false)));
             setKeyPool.insert(nIndex);
         }
         LogPrintf("CWallet::NewKeyPool wrote %d new keys\n", nKeys);
@@ -5073,7 +5079,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
             int64_t nEnd = 1;
             if (!setKeyPool.empty())
                 nEnd = *(--setKeyPool.end()) + 1;
-            if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(true))))
+            if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(false))))
                 throw runtime_error("TopUpKeyPool(): writing generated key failed");
             setKeyPool.insert(nEnd);
             LogPrintf("keypool added key %d, size=%u\n", nEnd, setKeyPool.size());
@@ -5130,23 +5136,6 @@ void CWallet::ReturnKey(int64_t nIndex)
     LogPrintf("keypool return %d\n", nIndex);
 }
 
-std::optional<CPubKey> CWallet::GetKeyFromPool()
-{
-    int64_t nIndex = 0;
-    CKeyPool keypool;
-    {
-        LOCK(cs_wallet);
-        ReserveKeyFromKeyPool(nIndex, keypool);
-        if (nIndex == -1)
-        {
-            if (IsLocked()) return std::nullopt;
-            return GenerateNewKey(true);
-        }
-        KeepKey(nIndex);
-        return keypool.vchPubKey;
-    }
-}
-
 int64_t CWallet::GetOldestKeyPoolTime()
 {
     int64_t nIndex = 0;
@@ -5715,9 +5704,9 @@ bool CWallet::InitLoadWallet(const CChainParams& params, bool clearWitnessCaches
     if (fFirstRun)
     {
         // Create new keyUser and set as default key
-        std::optional<CPubKey> newDefaultKey = walletInstance->GetKeyFromPool();
+        if (!walletInstance->IsCrypted()) {
-        if (newDefaultKey.has_value()) {
+            CPubKey newDefaultKey = walletInstance->GenerateNewKey(true);
-            walletInstance->SetDefaultKey(newDefaultKey.value());
+            walletInstance->SetDefaultKey(newDefaultKey);
             if (!walletInstance->SetAddressBook(walletInstance->vchDefaultKey.GetID(), "", "receive"))
                 return UIError(_("Cannot write default address") += "\n");
         }

src/wallet/wallet.h

@@ -1531,12 +1531,13 @@ public:
      */
     static CAmount GetRequiredFee(unsigned int nTxBytes);
 
+private:
     bool NewKeyPool();
+public:
     bool TopUpKeyPool(unsigned int kpSize = 0);
     void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool);
     void KeepKey(int64_t nIndex);
     void ReturnKey(int64_t nIndex);
-    std::optional<CPubKey> GetKeyFromPool();
     int64_t GetOldestKeyPoolTime();
     void GetAllReserveKeys(std::set<CKeyID>& setAddress) const;
 

src/wallet/walletdb.h

@@ -49,7 +49,8 @@ private:
     uint256 seedFp;
     int64_t nCreateTime; // 0 means unknown
     uint32_t accountCounter;
-    uint32_t legacyTKeyCounter;
+    uint32_t legacyTKeyExternalCounter;
+    uint32_t legacyTKeyInternalCounter;
     uint32_t legacySaplingKeyCounter;
     bool mnemonicSeedBackupConfirmed;
 
@@ -61,7 +62,8 @@ private:
         seedFp.SetNull();
         nCreateTime = 0;
         accountCounter = 0;
-        legacyTKeyCounter = 0;
+        legacyTKeyExternalCounter = 0;
+        legacyTKeyInternalCounter = 0;
         legacySaplingKeyCounter = 0;
         mnemonicSeedBackupConfirmed = false;
     }
@@ -69,7 +71,7 @@ public:
     static const int VERSION_HD_BASE = 1;
     static const int CURRENT_VERSION = VERSION_HD_BASE;
 
-    CHDChain(uint256 seedFpIn, int64_t nCreateTimeIn): nVersion(CHDChain::CURRENT_VERSION), seedFp(seedFpIn), nCreateTime(nCreateTimeIn), accountCounter(0), legacyTKeyCounter(0), legacySaplingKeyCounter(0), mnemonicSeedBackupConfirmed(false) {}
+    CHDChain(uint256 seedFpIn, int64_t nCreateTimeIn): nVersion(CHDChain::CURRENT_VERSION), seedFp(seedFpIn), nCreateTime(nCreateTimeIn), accountCounter(0), legacyTKeyExternalCounter(0), legacyTKeyInternalCounter(0), legacySaplingKeyCounter(0), mnemonicSeedBackupConfirmed(false) {}
 
     ADD_SERIALIZE_METHODS;
 
@@ -80,7 +82,8 @@ public:
         READWRITE(seedFp);
         READWRITE(nCreateTime);
         READWRITE(accountCounter);
-        READWRITE(legacyTKeyCounter);
+        READWRITE(legacyTKeyExternalCounter);
+        READWRITE(legacyTKeyInternalCounter);
         READWRITE(legacySaplingKeyCounter);
         READWRITE(mnemonicSeedBackupConfirmed);
     }
@@ -105,12 +108,16 @@ public:
         accountCounter += 1;
     }
 
-    uint32_t GetLegacyTKeyCounter() {
+    uint32_t GetLegacyTKeyCounter(bool external) {
-        return legacyTKeyCounter;
+        return external ? legacyTKeyExternalCounter : legacyTKeyInternalCounter;
     }
 
-    void IncrementLegacyTKeyCounter() {
+    void IncrementLegacyTKeyCounter(bool external) {
-        legacyTKeyCounter += 1;
+        if (external) {
+            legacyTKeyExternalCounter += 1;
+        } else {
+            legacyTKeyInternalCounter += 1;
+        }
     }
 
     uint32_t GetLegacySaplingKeyCounter() const {