[Do not merge] Stop advancing best block and shutdown node if keypool drops below critical threshold

src/qt/addresstablemodel.cpp

@@ -369,17 +369,14 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
     {
         // Generate a new address to associate with given label
         CPubKey newKey;
-        if(!wallet->GetKeyFromPool(newKey))
-        {
+        if (!wallet->GetKeyFromPool(newKey, false, true)) {
             WalletModel::UnlockContext ctx(walletModel->requestUnlock());
-            if(!ctx.isValid())
-            {
+            if(!ctx.isValid()) {
                 // Unlock wallet failed or was cancelled
                 editStatus = WALLET_UNLOCK_FAILURE;
                 return QString();
             }
-            if(!wallet->GetKeyFromPool(newKey))
-            {
+            if (!wallet->GetKeyFromPool(newKey, false, true)) {
                 editStatus = KEY_GENERATION_FAILURE;
                 return QString();
             }

src/qt/paymentserver.cpp

@@ -648,7 +648,7 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, SendCoinsRecipient recipien
     }
     else {
         CPubKey newKey;
-        if (wallet->GetKeyFromPool(newKey)) {
+        if (wallet->GetKeyFromPool(newKey, false, true)) {
             CKeyID keyID = newKey.GetID();
             wallet->SetAddressBook(keyID, strAccount, "refund");
 

src/wallet/rpcwallet.cpp

@@ -161,8 +161,8 @@ UniValue getnewaddress(const JSONRPCRequest& request)
 
     // Generate a new key that is added to wallet
     CPubKey newKey;
-    if (!pwallet->GetKeyFromPool(newKey)) {
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
+    if (!pwallet->GetKeyFromPool(newKey, false, true)) {
+        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool is at critical level or has run out, please call keypoolrefill first");
     }
     CKeyID keyID = newKey.GetID();
 

src/wallet/wallet.cpp

@@ -417,6 +417,19 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
 
 void CWallet::SetBestChain(const CBlockLocator& loc)
 {
+    LOCK(cs_wallet); //nWalletMaxVersion
+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
+    if (GetBoolArg("-bypasskeypoolcritical", false) || 
+        (IsHDEnabled() && (!m_update_best_block || !HasUnusedKeys(keypool_min)))) {
+        // If the keypool has dropped below -keypoolmin, then don't update the bestblock height. We can rescan later once the wallet is unlocked.
+
+        if (m_update_best_block) {
+            LogPrintf("Keypool has fallen below keypool_min (%s). Wallet will no longer watch for new transactions and best block height will not be advanced.\n", keypool_min);
+            LogPrintf("Unlock wallet, top up keypool and rescan to resume watching for new transactions.\n");
+            m_update_best_block = false;
+        }
+        return;
+    }
     CWalletDB walletdb(*dbw);
     walletdb.WriteBestBlock(loc);
 }
@@ -872,8 +885,9 @@ bool CWallet::GetAccountPubkey(CPubKey &pubKey, std::string strAccount, bool bFo
 
     // Generate a new key
     if (bForceNew) {
-        if (!GetKeyFromPool(account.vchPubKey, false))
+        if (!GetKeyFromPool(account.vchPubKey, false, true)) {
             return false;
+        }
 
         SetAddressBook(account.vchPubKey.GetID(), strAccount, "receive");
         walletdb.WriteAccount(strAccount, account);
@@ -1644,6 +1658,27 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
 
         fScanningWallet = false;
     }
+
+    // Update m_update_best_block if we've scanned from before the previous best block and keypool level is still above Keypool_min
+    CWalletDB walletdb(*dbw);
+    CBlockLocator loc;
+    walletdb.ReadBestBlock(loc);
+    CBlockIndex* pindexBestBlock = FindForkInGlobalIndex(chainActive, loc);
+    if (IsHDEnabled() && pindexStart && pindexStart->nHeight <= pindexBestBlock->nHeight) {
+        m_update_best_block = HasUnusedKeys(GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN));
+    }
+
+    // If the keypool has dropped below -keypoolmin, then stop updating the bestblock height. We can rescan later once the wallet is unlocked.
+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
+    if (IsHDEnabled() && !HasUnusedKeys(keypool_min) && m_update_best_block) {
+        LogPrintf("Keypool has fallen below keypool_min (%s). Wallet will no longer watch for new transactions and best block height will not be advanced.\n", keypool_min);
+        LogPrintf("Unlock wallet, top up keypool and rescan to resume watching for new transactions.\n");
+        m_update_best_block = false;
+    }
+
+    // Check that we haven't dropped below the keypool_critical threshold.
+    ShutdownIfKeypoolCritical();
+
     return ret;
 }
 
@@ -3410,11 +3445,21 @@ void CWallet::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)
     LogPrintf("keypool return %d\n", nIndex);
 }
 
-bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
+bool CWallet::GetKeyFromPool(CPubKey& result, bool internal, bool fail_on_critical)
 {
     CKeyPool keypool;
     {
         LOCK(cs_wallet);
+
+        // First, try to top up the keypool (this is a no-op if wallet is encrypted and locked)
+        TopUpKeyPool();
+        if (fail_on_critical && IsHDEnabled() &&
+            !HasUnusedKeys(GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL) + 1)) {
+            // If getting a key from the pool would result in us dropping to
+            // the keypool_critical threshold, fail
+            return false;
+        }
+
         int64_t nIndex = 0;
         ReserveKeyFromKeyPool(nIndex, keypool, internal);
         if (nIndex == -1)
@@ -3641,6 +3686,29 @@ void CReserveKey::ReturnKey()
     vchPubKey = CPubKey();
 }
 
+void CWallet::ShutdownIfKeypoolCritical() {
+    LOCK(cs_wallet);
+    unsigned int keypool_critical = GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL);
+    if (!GetBoolArg("-bypasskeypoolcritical", false) &&
+        IsHDEnabled() &&
+        !HasUnusedKeys(keypool_critical)) {
+        // if the remaining keypool size is below the gap limit, shutdown
+        LogPrintf("%s: Number of keys in keypool is below critical minimum. Shutting down. internal keypool: %d, external keypool: %d, keypool minimum: %d\n",
+                  __func__, setInternalKeyPool.size(), setExternalKeyPool.size(), keypool_critical);
+        const std::string error_msg = "Number of keys in keypool is below critical minimum and the wallet is encrypted. Bitcoin will now shutdown to avoid loss of funds.\n"
+                                      "This is probably because you are restoring an old backup wallet file which has not been topped up with the most recently "
+                                      "derived keys, and so you would not detect transactions involving those keys.\n"
+                                      "You can manually top-up your wallet keypool as follows:\n"
+                                      " - restart bitcoin with -bypasskeypoolcritical (to prevent bitcoind from shutting down again)\n"
+                                      " - unlock the wallet using walletpassphrase (to top up the keypool)\n"
+                                      " - restart without -bypasskeypoolcritical.\n"
+                                      "NOTE: if you have a pruned node that prunes blocks your wallet hasn't scanned yet when restarting with -bypasskeypoolcritical "
+                                      "then you may need to do a complete reindex. Consider raising the prune limit temporarily for both restarts to avoid this.";
+        uiInterface.ThreadSafeMessageBox(error_msg, "", CClientUIInterface::MSG_ERROR);
+        StartShutdown();
+    }
+}
+
 void CWallet::MarkReserveKeysAsUsed(int64_t keypool_id)
 {
     AssertLockHeld(cs_wallet);
@@ -3919,8 +3987,11 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
     {
         strUsage += HelpMessageGroup(_("Wallet debugging/testing options:"));
 
+        strUsage += HelpMessageOpt("-bypasskeypoolcritical", _("Bypass keypool critical limit. Don't shutdown the node if keypool drops below keypoolcritical limit (but don't advance BestBlock)"));
         strUsage += HelpMessageOpt("-dblogsize=<n>", strprintf("Flush wallet database activity from memory to disk log every <n> megabytes (default: %u)", DEFAULT_WALLET_DBLOGSIZE));
         strUsage += HelpMessageOpt("-flushwallet", strprintf("Run a thread to flush wallet periodically (default: %u)", DEFAULT_FLUSHWALLET));
+        strUsage += HelpMessageOpt("-keypoolcritical", strprintf(_("If the keypool drops below this number of keys and we are unable to generate new keys, shutdown (default: %u)"), DEFAULT_KEYPOOL_CRITICAL));
+        strUsage += HelpMessageOpt("-keypoolmin", strprintf(_("If the keypool drops below this number of keys and we are unable to generate new keys, don't advance the wallet's best block (default: %u)"), DEFAULT_KEYPOOL_MIN));
         strUsage += HelpMessageOpt("-privdb", strprintf("Sets the DB_PRIVATE flag in the wallet db environment (default: %u)", DEFAULT_WALLET_PRIVDB));
         strUsage += HelpMessageOpt("-walletrejectlongchains", strprintf(_("Wallet will not create transactions that violate mempool chain limits (default: %u)"), DEFAULT_WALLET_REJECT_LONG_CHAINS));
     }
@@ -4041,6 +4112,17 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
 
     RegisterValidationInterface(walletInstance);
 
+    // Make sure we have enough keys in our keypool if HD is enabled
+    if (walletInstance->IsHDEnabled()) {
+        unsigned int keypool_size = GetArg("-keypool", DEFAULT_KEYPOOL_SIZE);
+        unsigned int keypool_critical = GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL);
+        if (keypool_size < keypool_critical) {
+            LogPrintf("Parameter Interaction: keypool critical size (%d) must be smaller than keypool size (%d)\n", keypool_critical, keypool_size);
+            ForceSetArg("-keypool", std::to_string(keypool_critical));
+        }
+
+    }
+
     // Try to top up keypool. No-op if the wallet is locked.
     walletInstance->TopUpKeyPool();
 

src/wallet/wallet.h

@@ -40,7 +40,15 @@ extern unsigned int nTxConfirmTarget;
 extern bool bSpendZeroConfChange;
 extern bool fWalletRbf;
 
+//! Default size for the keypool. Always try to top up the keypool to have this
+//  many keys. For an HD split wallet, have this many keys in each of the
+//  internal/external chains
 static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000;
+//! Don't update wallet's best block if keypool falls below this size (to avoid
+//  not detecting transactions)
+static const unsigned int DEFAULT_KEYPOOL_MIN = 500;
+//! Shut down if the keypool falls below this size
+static const unsigned int DEFAULT_KEYPOOL_CRITICAL = 500;
 //! -paytxfee default
 static const CAmount DEFAULT_TRANSACTION_FEE = 0;
 //! -fallbackfee default
@@ -657,6 +665,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     static std::atomic<bool> fFlushScheduled;
     std::atomic<bool> fAbortRescan;
     std::atomic<bool> fScanningWallet;
+    bool m_update_best_block;
 
     /**
      * Select a set of coins such that nValueRet >= nTargetValue and at least
@@ -792,6 +801,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
         nRelockTime = 0;
         fAbortRescan = false;
         fScanningWallet = false;
+        m_update_best_block = true;
     }
 
     std::map<uint256, CWalletTx> mapWallet;
@@ -977,8 +987,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
     void KeepKey(int64_t nIndex);
     void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey);
-    bool GetKeyFromPool(CPubKey &key, bool internal = false);
+    bool GetKeyFromPool(CPubKey &key, bool internal = false, bool fail_on_critical = false);
     int64_t GetOldestKeyPoolTime();
+    void ShutdownIfKeypoolCritical();
     /**
      * Marks all keys in the keypool up to and including reserve_key as used.
      */

test/functional/keypool-topup.py

@@ -6,15 +6,23 @@
 
 Two nodes. Node1 is under test. Node0 is providing transactions and generating blocks.
 
+Repeat test twice, once with encrypted wallet and once with unencrypted wallet:
+
 - Start node1, shutdown and backup wallet.
 - Generate 110 keys (enough to drain the keypool). Store key 90 (in the initial keypool) and key 110 (beyond the initial keypool). Send funds to key 90 and key 110.
 - Stop node1, clear the datadir, move wallet file back into the datadir and restart node1.
-- connect node1 to node0. Verify that they sync and node1 receives its funds."""
+- if wallet is unencrypted:
+    - connect node1 to node0. Verify that they sync and node1 receives its funds.
+- else wallet is encrypted:
+    - connect node1 to node0. Verify that node1 shuts down to avoid loss of funds.
+"""
+import os
 import shutil
 
-from test_framework.test_framework import BitcoinTestFramework
+from test_framework.test_framework import BitcoinTestFramework, BITCOIND_PROC_WAIT_TIMEOUT
 from test_framework.util import (
     assert_equal,
+    assert_raises_jsonrpc,
     connect_nodes_bi,
     sync_blocks,
 )
@@ -24,23 +32,41 @@ def __init__(self):
         super().__init__()
         self.setup_clean_chain = True
         self.num_nodes = 2
-        self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolmin=20']]
+        self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolmin=20', '-keypoolcritical=20']]
 
     def run_test(self):
         self.tmpdir = self.options.tmpdir
         self.nodes[0].generate(101)
 
+        self.log.info("Test keypool restore (encrypted wallet)")
+        self._test_restore(encrypted=True)
+
+        self.log.info("Test keypool restore (unencrypted wallet)")
+        self._test_restore(encrypted=False)
+
+    def _test_restore(self, encrypted):
+
         self.log.info("Make backup of wallet")
 
-        self.stop_node(1)
+        if encrypted:
+            self.nodes[1].encryptwallet('test')
+            self.bitcoind_processes[1].wait(timeout=BITCOIND_PROC_WAIT_TIMEOUT)
+        else:
+            self.stop_node(1)
 
         shutil.copyfile(self.tmpdir + "/node1/regtest/wallet.dat", self.tmpdir + "/wallet.bak")
         self.nodes[1] = self.start_node(1, self.tmpdir, self.extra_args[1])
         connect_nodes_bi(self.nodes, 0, 1)
 
         self.log.info("Generate keys for wallet")
 
-        for _ in range(90):
+        for _ in range(80):
+            self.nodes[1].getnewaddress()
+        if encrypted:
+            # Keypool can't top up because the wallet is encrypted
+            assert_raises_jsonrpc(-12, "Keypool is at critical level or has run out, please call keypoolrefill first", self.nodes[1].getnewaddress)
+            self.nodes[1].walletpassphrase("test", 10)
+        for _ in range(10):
             addr_oldpool = self.nodes[1].getnewaddress()
         for _ in range(20):
             addr_extpool = self.nodes[1].getnewaddress()
@@ -59,17 +85,28 @@ def run_test(self):
 
         shutil.copyfile(self.tmpdir + "/wallet.bak", self.tmpdir + "/node1/regtest/wallet.dat")
 
-        self.log.info("Verify keypool is restored and balance is correct")
+        if encrypted:
+            self.log.info("Encrypted wallet - verify node shuts down to avoid loss of funds")
 
-        self.nodes[1] = self.start_node(1, self.tmpdir, self.extra_args[1])
-        connect_nodes_bi(self.nodes, 0, 1)
-        self.sync_all()
+            self.assert_start_raises_init_error(1, self.tmpdir, self.extra_args[1], expected_msg="Number of keys in keypool is below critical minimum and the wallet is encrypted. Bitcoin will now shutdown to avoid loss of funds.")
+
+            shutil.rmtree(self.tmpdir + "/node1/regtest/chainstate")
+            shutil.rmtree(self.tmpdir + "/node1/regtest/blocks")
+            os.remove(self.tmpdir + "/node1/regtest/wallet.dat")
+            self.nodes[1] = self.start_node(1, self.tmpdir, self.extra_args[1])
+
+        else:
+            self.log.info("Unencrypted wallet - verify keypool is restored and balance is correct")
+
+            self.nodes[1] = self.start_node(1, self.tmpdir, self.extra_args[1])
+            connect_nodes_bi(self.nodes, 0, 1)
+            self.sync_all()
 
-        assert_equal(self.nodes[1].getbalance(), 15)
-        assert_equal(self.nodes[1].listtransactions()[0]['category'], "receive")
+            assert_equal(self.nodes[1].getbalance(), 15)
+            assert_equal(self.nodes[1].listtransactions()[0]['category'], "receive")
 
-        # Check that we have marked all keys up to the used keypool key as used
-        assert_equal(self.nodes[1].validateaddress(self.nodes[1].getnewaddress())['hdkeypath'], "m/0'/0'/111'")
+            # Check that we have marked all keys up to the used keypool key as used
+            assert_equal(self.nodes[1].validateaddress(self.nodes[1].getnewaddress())['hdkeypath'], "m/0'/0'/111'")
 
 if __name__ == '__main__':
     KeypoolRestoreTest().main()

test/functional/keypool.py

@@ -27,7 +27,7 @@ def run_test(self):
         wallet_info = nodes[0].getwalletinfo()
         assert(addr_before_encrypting_data['hdmasterkeyid'] != wallet_info['hdmasterkeyid'])
         assert(addr_data['hdmasterkeyid'] == wallet_info['hdmasterkeyid'])
-        assert_raises_jsonrpc(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress)
+        assert_raises_jsonrpc(-12, "Keypool is at critical level or has run out, please call keypoolrefill first", nodes[0].getnewaddress)
 
         # put six (plus 2) new keys in the keypool (100% external-, +100% internal-keys, 1 in min)
         nodes[0].walletpassphrase('test', 12000)
@@ -44,20 +44,11 @@ def run_test(self):
         nodes[0].getrawchangeaddress()
         nodes[0].getrawchangeaddress()
         nodes[0].getrawchangeaddress()
-        addr = set()
         # the next one should fail
         assert_raises_jsonrpc(-12, "Keypool ran out", nodes[0].getrawchangeaddress)
 
-        # drain the external keys
-        addr.add(nodes[0].getnewaddress())
-        addr.add(nodes[0].getnewaddress())
-        addr.add(nodes[0].getnewaddress())
-        addr.add(nodes[0].getnewaddress())
-        addr.add(nodes[0].getnewaddress())
-        addr.add(nodes[0].getnewaddress())
-        assert(len(addr) == 6)
-        # the next one should fail
-        assert_raises_jsonrpc(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress)
+        # getting an external key should also fail now
+        assert_raises_jsonrpc(-12, "Keypool is at critical level or has run out, please call keypoolrefill first", nodes[0].getnewaddress)
 
         # refill keypool with three new addresses
         nodes[0].walletpassphrase('test', 1)
@@ -68,9 +59,8 @@ def run_test(self):
         assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0)
 
         # drain them by mining
-        nodes[0].generate(1)
-        nodes[0].generate(1)
-        nodes[0].generate(1)
+        for _ in range(6):
+            nodes[0].generate(1)
         assert_raises_jsonrpc(-12, "Keypool ran out", nodes[0].generate, 1)
 
         nodes[0].walletpassphrase('test', 100)