CORE: Fix an issue with mining address changing when doing a restore

Mining address should never change for a mining accounts.
Fixes #201

src/account.cpp

@@ -425,6 +425,18 @@ CAccountHD::CAccountHD(CExtPubKey accountKey_, boost::uuids::uuid seedID, Accoun
     m_readOnly = true;
 }
 
+bool CAccountHD::GetKeyIDWithHighestIndex(CKeyID& HDKeyID, int nChain) const
+{
+    if (nChain == KEYCHAIN_EXTERNAL)
+    {
+        return externalKeyStore.GetKeyIDWithHighestIndex(HDKeyID);
+    }
+    else
+    {
+        return internalKeyStore.GetKeyIDWithHighestIndex(HDKeyID);
+    }
+}
+
 bool CAccountHD::GetKey(CExtKey& childKey, int nChain) const
 {
     assert(!m_readOnly);
@@ -1073,13 +1085,25 @@ uint64_t CAccount::getEarliestPossibleCreationTime()
     return earliestPossibleCreationTime;
 }
 
-//fixme: (FUT) (ACCOUNTS) (CLEANUP) - relook at how the wallet reports keypool size in an accounts context.
 unsigned int CAccount::GetKeyPoolSize()
 {
     AssertLockHeld(cs_keypool); // setKeyPool
     return setKeyPoolExternal.size();
 }
 
+unsigned int CAccount::GetKeyPoolSize(int nChain)
+{
+    AssertLockHeld(cs_keypool); // setKeyPool
+    if (nChain == KEYCHAIN_EXTERNAL)
+    {
+        return setKeyPoolExternal.size();
+    }
+    else
+    {
+        return setKeyPoolInternal.size();
+    }
+}
+
 std::string CAccount::getLabel() const
 {
     return accountLabel;

src/account.h

@@ -360,7 +360,8 @@ class CAccount : public CCryptoKeyStore
     virtual bool AddKeyPubKey(int64_t HDKeyIndex, const CPubKey &pubkey, int keyChain);
     void AddChild(CAccount* childAccount);
 
-    unsigned int GetKeyPoolSize();
+    unsigned int GetKeyPoolSize(int nchain);
+    [[deprecated]] unsigned int GetKeyPoolSize();
 
     std::string getLabel() const;
     void setLabel(const std::string& label, CWalletDB* Db);
@@ -435,6 +436,7 @@ class CAccountHD: public CAccount
     //For serialization only.
     CAccountHD(){};
 
+    virtual bool GetKeyIDWithHighestIndex(CKeyID& HDKeyID, int nChain) const;
     virtual bool GetKey(CExtKey& childKey, int nChain) const;
     virtual bool GetKey(const CKeyID& keyID, CKey& key) const override;
     virtual bool GetKey(const CKeyID &address, std::vector<unsigned char>& encryptedKeyOut) const override;

src/coins.cpp

@@ -125,7 +125,19 @@ void CCoinsViewCache::validateInsert(const COutPoint &outpoint, uint64_t block,
         else
         {
             // no entry in cacheCoinRefs, so it should be absent in cacheCoins also
-            assert(!outpoint.isHash || cacheCoins.find(outpoint) == cacheCoins.end());
+            if (outpoint.isHash)
+            {
+                if (cacheCoins.find(outpoint) != cacheCoins.end())
+                {
+                    CCoinsMap::iterator it = cacheCoins.find(outpoint);
+                    std::string warning = strprintf("Warning: outpoint mismatch.\nPlease notify the developers with this information to assist them.\n\n"
+                                                "ophash:[%s]\nblock:[%d] txidx:[%d] outidx:[%d] flags:[%d] spent:[%s] lookupishash: [%s]",
+                                                outpoint.getTransactionHash().ToString(), 
+                                                block, txIndex, voutIndex, it->second.flags, (it->second.coin.IsSpent()?"yes":"no"), (outpoint.isHash?"yes":"no"));
+                    uiInterface.NotifyUIAlertChanged(warning);
+                }
+            }
+            // The non-hash output definitely shouldn't be in the cacheCoins ever
             assert(outpoint.isHash || cacheCoins.find(COutPoint(block, txIndex, voutIndex)) == cacheCoins.end());
         }
     }

src/keystore.h

@@ -139,6 +139,24 @@ class CBasicKeyStore : public CKeyStore
         }
         return false;
     }
+    bool GetKeyIDWithHighestIndex(CKeyID& HDKeyID) const
+    {
+        int64_t highestIndex=0;
+        {
+            LOCK(cs_KeyStore);
+            if (mapHDKeys.empty())
+                return false;
+            for (const auto& [keyID, keyIndex] : mapHDKeys)
+            {
+                if (keyIndex >= highestIndex)
+                {
+                    highestIndex = keyIndex;
+                    HDKeyID = keyID;
+                }
+            }
+        }
+        return true;
+    }
     virtual bool AddCScript(const CScript& redeemScript);
     virtual bool HaveCScript(const CScriptID &hash) const;
     virtual bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const;

src/unity/unity_impl.cpp

@@ -205,18 +205,23 @@ TransactionRecord calculateTransactionRecordForWalletTransaction(const CWalletTx
 
     //fixme: (UNITY) - rather calculate this once and pass it in instead of for every call..
     std::vector<CKeyStore*> accountsToTry;
-    for ( const auto& accountPair : pactiveWallet->mapAccounts )
+    accountsToTry.push_back(pactiveWallet->activeAccount);
+    for (const auto& [accountUUID, account] : pactiveWallet->mapAccounts)
     {
-        if(accountPair.second->getParentUUID() == pactiveWallet->activeAccount->getUUID())
+        (unused) accountUUID;
+        if (account->getParentUUID() == pactiveWallet->activeAccount->getUUID())
         {
-            accountsToTry.push_back(accountPair.second);
+            accountsToTry.push_back(account);
         }
-        accountsToTry.push_back(pactiveWallet->activeAccount);
     }
-
 
-    int64_t subtracted = wtx.GetDebit(ISMINE_SPENDABLE, pactiveWallet->activeAccount, true);
-    int64_t added = wtx.GetCredit(ISMINE_SPENDABLE, pactiveWallet->activeAccount, true);
+    int64_t subtracted;
+    int64_t added;
+    for (const auto& account : accountsToTry)
+    {
+        subtracted += wtx.GetDebit(ISMINE_SPENDABLE, account, true);
+        added += wtx.GetCredit(ISMINE_SPENDABLE, account, true);
+    }
 
     CAmount fee = 0;
     // if any funds were subtracted the transaction was sent by us

src/wallet/Gulden/guldenwallet.cpp

@@ -357,6 +357,23 @@ void CGuldenWallet::MarkKeyUsed(CKeyID keyID, uint64_t usageTime)
                 {
                     keyUsedSet.insert(keyID);
 
+                    if (forAccount->IsMiningAccount())
+                    {
+                        CKeyID highestKeyID;
+                        //fixme: (HIGH) (KEYPOOL) All HD accounts should use a similar (not identical) method to this
+                        //To obtain a proper key gap instead of what we have now
+                        //Probably we should be storing the 'highest used key' somewhere in this function
+                        //And then topupkeypool should utilise it for allocation (instead of the allocation trick we use here)
+                        if (dynamic_cast<CAccountHD*>(forAccount)->GetKeyIDWithHighestIndex(highestKeyID, KEYCHAIN_EXTERNAL) && (highestKeyID == keyID))
+                        {
+                            // Assign 1 extra key, because mining accounts never discard keys the keypool size always grows when new keys are allocated
+                            // Ideally most mining accounts will only have 1 key, but due to a previous bug some have more
+                            LOCK(forAccount->cs_keypool);
+                            uint64_t topupSize = forAccount->GetKeyPoolSize(KEYCHAIN_EXTERNAL)+1;
+                            static_cast<CWallet*>(this)->TopUpKeyPool(topupSize, 10, forAccount, topupSize);
+                        }
+                    }
+
                     if (forAccount->m_State != AccountState::Normal && forAccount->m_State != AccountState::ShadowChild)
                     {
                         forAccount->m_State = AccountState::Normal;

src/wallet/crypter.cpp

@@ -319,6 +319,15 @@ bool CCryptoKeyStore::GetKey(const CKeyID &address, std::vector<unsigned char>&
     return false;
 }
 
+bool CCryptoKeyStore::GetKeyIDWithHighestIndex(CKeyID &address) const
+{
+   // For HD we don't encrypt anything here - as the public key we need access to anyway, and the index is not special info - we derive the private key when we need it.
+    {
+        LOCK(cs_KeyStore);
+        return CBasicKeyStore::GetKeyIDWithHighestIndex(address);
+    }
+}
+
 bool CCryptoKeyStore::GetKey(const CKeyID &address, CKey& keyOut) const
 {
     {

src/wallet/crypter.h

@@ -187,6 +187,7 @@ class CCryptoKeyStore : public CBasicKeyStore
     }
     virtual bool GetKey(const CKeyID &address, std::vector<unsigned char>& encryptedKeyOut) const;
     virtual bool GetKey(const CKeyID &address, CKey& keyOut) const;
+    virtual bool GetKeyIDWithHighestIndex(CKeyID& address) const;
     virtual bool GetKey(const CKeyID &address, int64_t& HDKeyIndex) const;
     virtual bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const;
     virtual void GetKeys(std::set<CKeyID> &setAddress) const

src/wallet/wallet.h

@@ -826,7 +826,7 @@ class CWallet : public CGuldenWallet
     /*
     size_t KeypoolCountExternalKeys();
     */
-    int TopUpKeyPool(unsigned int nTargetKeypoolSize = 0, unsigned int nMaxNewAllocations = 0, CAccount* forAccount = nullptr);
+    int TopUpKeyPool(unsigned int nTargetKeypoolSize = 0, unsigned int nMaxNewAllocations = 0, CAccount* forAccount = nullptr, unsigned int nMinimalKeypoolOverride = 1);
     void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, CAccount* forAccount, int64_t keyChain);
     void KeepKey(int64_t nIndex);
     void ReturnKey(int64_t nIndex, CAccount* forAccount, int64_t keyChain);

src/wallet/wallet_keypool.cpp

@@ -59,7 +59,7 @@ bool CWallet::NewKeyPool()
 }
 
 //fixme: (FUT) (ACCOUNTS) GULDEN Note for HD this should actually care more about maintaining a gap above the last used address than it should about the size of the pool.
-int CWallet::TopUpKeyPool(unsigned int nTargetKeypoolSize, unsigned int nMaxNewAllocations, CAccount* topupForAccount)
+int CWallet::TopUpKeyPool(unsigned int nTargetKeypoolSize, unsigned int nMaxNewAllocations, CAccount* topupForAccount, unsigned int nMinimalKeypoolOverride)
 {
     // Return -1 if we fail to allocate any -and- one of the accounts is not HD -and- it is locked.
     uint32_t nNew = 0;
@@ -103,7 +103,7 @@ int CWallet::TopUpKeyPool(unsigned int nTargetKeypoolSize, unsigned int nMaxNewA
                 uint32_t nFinalAccountTargetSize = nAccountTargetSize;
                 if (loopForAccount->IsMinimalKeyPool())
                 {
-                    nFinalAccountTargetSize = 1;
+                    nFinalAccountTargetSize = nMinimalKeypoolOverride;
                 }
 
                 for (auto& keyChain : { KEYCHAIN_EXTERNAL, KEYCHAIN_CHANGE })

src/wallet/walletdb.cpp

@@ -284,8 +284,7 @@ bool CWalletDB::ErasePool(CWallet* pwallet, const CKeyID& id, bool forceErase)
             {
                 if ((iter.second->setKeyPoolExternal.find(keyIndex) != iter.second->setKeyPoolExternal.end()) || (iter.second->setKeyPoolInternal.find(keyIndex) != iter.second->setKeyPoolInternal.end()))
                 {
-                    allowErase = false;
-                    break;
+                    return true;
                 }
             }
         }
@@ -295,11 +294,8 @@ bool CWalletDB::ErasePool(CWallet* pwallet, const CKeyID& id, bool forceErase)
     //Remove from internal keypool, key has been used so shouldn't circulate anymore - address will now reside only in address book.
     for (auto iter : pwallet->mapAccounts)
     {
-        if (forceErase || !iter.second->IsFixedKeyPool())
-        {
-            iter.second->setKeyPoolExternal.erase(keyIndex);
-            iter.second->setKeyPoolInternal.erase(keyIndex);
-        }
+        iter.second->setKeyPoolExternal.erase(keyIndex);
+        iter.second->setKeyPoolInternal.erase(keyIndex);
     }
 
     //Remove from disk