Merge bitcoin#1401: [Wallet][Bug] Fix ScriptPubKeyMan::CanGetAddresses

5f21fc4 [Wallet] Properly return keys to external pool for non-HD wallets (random-zebra)
998cdb2 [Wallet][Bug] Fix ScriptPubKeyMan::CanGetAddresses (random-zebra)

Pull request description:

  This fixes a bug found in the recent HD wallet implementation.
  Trying to send any transaction (included delegations) on non-upgraded wallets results in a crash due to the following assertion failure in `CWallet::CreateTransaction`:
  ```cpp
  // Reserve a new key pair from key pool
  CPubKey vchPubKey;
  bool ret;
  ret = reservekey.GetReservedKey(vchPubKey, true);
  assert(ret); // should never fail, as we just unlocked
  ```

  **Cause**: `assert(ret)` fails here because `GetReservedKey` returns false on non-HD wallets requesting change addresses (or stake addresses) because, in this case, `CanGetAddresses` returns false: both `setInternalKeyPool` and `setStakingKeyPool` are empty.

  **Fix**: on wallets with HD disabled, `CanGetAddresses`  should call `KeypoolCountExternalKeys` (which checks the size of `set_external_keypool`) with any changeType, as that's the pool used then in `ReserveKeyFromKeyPool`.

ACKs for top commit:
  Fuzzbawls:
    utACK 5f21fc4
  furszy:
    ACK 5f21fc4 and merging.

Tree-SHA512: 65b7291a169ed617a5c148f137aa74a2f93509a8a65adb3d777efd7a5109ca7e0103612c24bea8b98c5262d018d53b96c68016d990b9ec5bf9ace031237e88c6

src/wallet/scriptpubkeyman.cpp

@@ -65,13 +65,15 @@ bool ScriptPubKeyMan::CanGetAddresses(const uint8_t& type)
 {
     LOCK(wallet->cs_wallet);
     // Check if the keypool has keys
+    const bool isHDEnabled = IsHDEnabled();
     bool keypool_has_keys = false;
-    if (type == HDChain::ChangeType::INTERNAL && IsHDEnabled()) {
+    if (isHDEnabled && type == HDChain::ChangeType::INTERNAL) {
         keypool_has_keys = setInternalKeyPool.size() > 0;
-    } else if (type == HDChain::ChangeType::EXTERNAL) {
-        keypool_has_keys = KeypoolCountExternalKeys() > 0;
-    } else if (type == HDChain::ChangeType::STAKING) {
+    } else if (isHDEnabled && type == HDChain::ChangeType::STAKING) {
         keypool_has_keys = setStakingKeyPool.size() > 0;
+    } else {
+        // either external key was requested or HD is not enabled
+        keypool_has_keys = KeypoolCountExternalKeys() > 0;
     }
     // If the keypool doesn't have keys, check if we can generate them
     if (!keypool_has_keys) {
@@ -159,10 +161,10 @@ bool ScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const uint8_t& changeType)
 bool ScriptPubKeyMan::GetReservedKey(const uint8_t& changeType, int64_t& index, CKeyPool& keypool)
 {
     if (!CanGetAddresses(changeType)) {
-        return false;
+        return error("%s : Cannot get address for change type %d", __func__, changeType);
     }
     if (!ReserveKeyFromKeyPool(index, keypool, changeType)) {
-        return false;
+        return error("%s : Cannot reserve key from pool for change type %d", __func__, changeType);
     }
     return true;
 }
@@ -237,15 +239,15 @@ void ScriptPubKeyMan::ReturnDestination(int64_t nIndex, const uint8_t& type, con
     // Return to key pool
     {
         LOCK(wallet->cs_wallet);
-        bool fInternal = type == HDChain::ChangeType::INTERNAL;
-        if (fInternal) {
+        const bool isHDEnabled = IsHDEnabled();
+        if (isHDEnabled && type == HDChain::ChangeType::INTERNAL) {
             setInternalKeyPool.insert(nIndex);
-        } else if (!set_pre_split_keypool.empty()) {
+        } else if (isHDEnabled && type == HDChain::ChangeType::STAKING) {
+            setStakingKeyPool.insert(nIndex);
+        } else if (isHDEnabled && !set_pre_split_keypool.empty()) {
             set_pre_split_keypool.insert(nIndex);
-        } else if (type == HDChain::ChangeType::EXTERNAL) {
+        } else {
             setExternalKeyPool.insert(nIndex);
-        } else if (type == HDChain::ChangeType::STAKING) {
-            setStakingKeyPool.insert(nIndex);
         }
         CKeyID& pubkey_id = m_index_to_reserved_key.at(nIndex);
         m_pool_key_to_index[pubkey_id] = nIndex;
@@ -380,8 +382,9 @@ bool ScriptPubKeyMan::TopUp(unsigned int kpSize)
         int64_t missingStaking = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - (int64_t)setStakingKeyPool.size(), (int64_t) 0);
 
         if (!IsHDEnabled()) {
-            // don't create extra internal keys
+            // don't create extra internal or staking keys
             missingInternal = 0;
+            missingStaking = 0;
         }
 
         CWalletDB batch(wallet->strWalletFile);
@@ -418,16 +421,13 @@ void ScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const uint8_
         throw std::runtime_error(std::string(__func__) + ": writing imported pubkey failed");
     }
 
-    switch (type) {
-        case HDChain::ChangeType::EXTERNAL:
-            setExternalKeyPool.insert(index);
-            break;
-        case HDChain::ChangeType::INTERNAL:
-            setInternalKeyPool.insert(index);
-            break;
-        case HDChain::ChangeType::STAKING:
-            setStakingKeyPool.insert(index);
-            break;
+    const bool isHDEnabled = IsHDEnabled();
+    if (isHDEnabled && type == HDChain::ChangeType::INTERNAL) {
+        setInternalKeyPool.insert(index);
+    } else if (isHDEnabled && type == HDChain::ChangeType::STAKING) {
+        setStakingKeyPool.insert(index);
+    } else {
+        setExternalKeyPool.insert(index);
     }
 
     m_pool_key_to_index[pubkey.GetID()] = index;
@@ -684,4 +684,4 @@ void ScriptPubKeyMan::SetHDChain(CHDChain& chain, bool memonly)
     // Sanity check
     if (!wallet->HaveKey(hdChain.GetID()))
         throw std::runtime_error(std::string(__func__) + ": Not found seed in wallet");
-}
+}