Merge #13252: Wallet: Refactor ReserveKeyFromKeyPool for safety

Summary:
4b62bdf5136c174621509bf7866fbd89b61cc66a Wallet: Refactor ReserveKeyFromKeyPool for safety (Ben Woosley)

Pull request description:

  ReserveKeyFromKeyPool's previous behaviour is to set nIndex to -1 if the keypool is
  empty, OR throw an exception for technical failures. Instead, we now return false
  if the keypool is empty, true if the operation succeeded.

  This is to make failure more easily detectable by calling code.

Tree-SHA512: 753f057ad13bd4c28d121f426bf0967ed72b827d97fb24582f9326ec60072abc5482e3db69ccada7c5fc66de9957fc59098432dd223fc4116991cab44c6d7aef

Backport of Core PR13252
bitcoin/bitcoin#13252

Depends on D4207

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4166

Author: laanwj

src/wallet/wallet.cpp

@@ -4135,7 +4135,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) {
     return true;
 }
 
-void CWallet::ReserveKeyFromKeyPool(int64_t &nIndex, CKeyPool &keypool,
+bool CWallet::ReserveKeyFromKeyPool(int64_t &nIndex, CKeyPool &keypool,
                                     bool fRequestedInternal) {
     nIndex = -1;
     keypool.vchPubKey = CPubKey();
@@ -4146,12 +4146,11 @@ void CWallet::ReserveKeyFromKeyPool(int64_t &nIndex, CKeyPool &keypool,
         TopUpKeyPool();
     }
     bool fReturningInternal = fRequestedInternal;
-    std::set<int64_t> &setKeyPool =
-        fReturningInternal ? setInternalKeyPool : setExternalKeyPool;
+    std::set<int64_t> &setKeyPool =  (fReturningInternal) ? setInternalKeyPool : setExternalKeyPool;
 
     // Get the oldest key
     if (setKeyPool.empty()) {
-        return;
+        return false;
     }
 
     WalletBatch batch(*database);
@@ -4178,13 +4177,21 @@ void CWallet::ReserveKeyFromKeyPool(int64_t &nIndex, CKeyPool &keypool,
     } else {
         m_pool_key_to_index.erase(keypool.vchPubKey.GetKeyID());
     }
-
-
-    if (keypool.fInternal != fReturningInternal) {
+    // If the key was pre-split keypool, we don't care about what type it is
+    /*
+    if (use_split_keypool && keypool.fInternal != fReturningInternal) {
         throw std::runtime_error(std::string(__func__) +
                                  ": keypool entry misclassified");
     }
+    */
+    if (!keypool.vchPubKey.IsValid()) {
+        throw std::runtime_error(std::string(__func__) +
+                                 ": keypool entry invalid");
+    }
+
     LogPrintf("keypool reserve %d\n", nIndex);
+
+    return true;
 }
 
 void CWallet::KeepKey(int64_t nIndex) {
@@ -4215,9 +4222,8 @@ void CWallet::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey &pubkey) {
 bool CWallet::GetKeyFromPool(CPubKey &result, bool internal) {
     CKeyPool keypool;
     LOCK(cs_wallet);
-    int64_t nIndex = 0;  
-    ReserveKeyFromKeyPool(nIndex, keypool, internal);
-    if (nIndex == -1) {
+    int64_t nIndex;
+    if (!ReserveKeyFromKeyPool(nIndex, keypool, internal)) {
         if (IsLocked()) {
             return false;
         }
@@ -4460,8 +4466,7 @@ CWallet::GetLabelAddresses(const std::string &label) const {
 bool CReserveKey::GetReservedKey(CPubKey &pubkey, bool internal) {
     if (nIndex == -1) {
         CKeyPool keypool;
-        pwallet->ReserveKeyFromKeyPool(nIndex, keypool, internal);
-        if (nIndex == -1) {
+        if (!pwallet->ReserveKeyFromKeyPool(nIndex, keypool, internal)) {
             return false;
         }
 

src/wallet/wallet.h

@@ -1075,7 +1075,22 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface {
     bool NewKeyPool();
     size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool TopUpKeyPool(unsigned int kpSize = 0);
-    void ReserveKeyFromKeyPool(int64_t &nIndex, CKeyPool &keypool,
+
+    /**
+     * Reserves a key from the keypool and sets nIndex to its index
+     *
+     * @param[out] nIndex the index of the key in keypool
+     * @param[out] keypool the keypool the key was drawn from, which could be
+     * the the pre-split pool if present, or the internal or external pool
+     * @param fRequestedInternal true if the caller would like the key drawn
+     *     from the internal keypool, false if external is preferred
+     *
+     * @return true if succeeded, false if failed due to empty keypool
+     * @throws std::runtime_error if keypool read failed, key was invalid,
+     *     was not found in the wallet, or was misclassified in the internal
+     *     or external keypool
+     */
+    bool ReserveKeyFromKeyPool(int64_t &nIndex, CKeyPool &keypool,
                                bool fRequestedInternal);
     void KeepKey(int64_t nIndex);
     void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey &pubkey);