Fix wallet races and crashes

Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>

src/wallet/test/claim_rpc_tests.cpp

@@ -93,6 +93,15 @@ UniValue LookupAllNames() {
     return rpc_method(req);
 }
 
+UniValue PruneAbandonFunds(const uint256& txid) {
+    rpcfn_type rpc_method = tableRPC["removeprunedfunds"]->actor;
+    JSONRPCRequest req;
+    req.URI = "/wallet/tester_wallet";
+    req.params = UniValue(UniValue::VARR);
+    req.params.push_back(txid.GetHex());
+    return rpc_method(req);
+}
+
 std::vector<uint256> generateBlock(int blocks = 1) {
     rpcfn_type rpc_method = tableRPC["generate"]->actor;
     JSONRPCRequest req;
@@ -488,5 +497,17 @@ BOOST_AUTO_TEST_CASE(can_claim_after_each_fork)
     BOOST_CHECK_EQUAL(looked.size(), 4U);
 }
 
+BOOST_AUTO_TEST_CASE(remove_pruned_funds)
+{
+    generateBlock(140);
+    // claim a name
+    auto txid = ClaimAName("tester", "deadbeef", "1.0");
+    generateBlock();
+    // abandon claim
+    AbandonAClaim(txid);
+    generateBlock();
+    PruneAbandonFunds(txid);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 

src/wallet/wallet.cpp

@@ -44,7 +44,7 @@ bool AddWallet(const std::shared_ptr<CWallet>& wallet)
 {
     LOCK(cs_wallets);
     assert(wallet);
-    std::vector<std::shared_ptr<CWallet>>::const_iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet);
+    auto i = std::find(vpwallets.begin(), vpwallets.end(), wallet);
     if (i != vpwallets.end()) return false;
     vpwallets.push_back(wallet);
     return true;
@@ -92,8 +92,8 @@ static void ReleaseWallet(CWallet* wallet)
     // so that it's in sync with the current chainstate.
     wallet->WalletLogPrintf("Releasing wallet\n");
     wallet->BlockUntilSyncedToCurrentChain();
-    wallet->Flush();
     UnregisterValidationInterface(wallet);
+    wallet->Flush();
     delete wallet;
     // Wallet is now released, notify UnloadWallet, if any.
     {
@@ -565,8 +565,6 @@ std::set<uint256> CWallet::GetConflicts(const uint256& txid) const
         return result;
     const CWalletTx& wtx = it->second;
 
-    std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range;
-
     for (const CTxIn& txin : wtx.tx->vin)
     {
         auto hitTx = mapTxSpends.find(txin.prevout.hash);
@@ -615,10 +613,11 @@ void CWallet::SyncMetaData(const COutPoint& outPoint)
         auto hitN = hitTx->second.find(outPoint.n);
         if (hitN != hitTx->second.end()) {
             for (auto &spend: hitN->second) {
-                const CWalletTx *wtx = &mapWallet.at(spend);
+                if (auto wtx = GetWalletTx(spend)) {
-                if (wtx->nOrderPos < nMinOrderPos) {
+                    if (wtx->nOrderPos < nMinOrderPos) {
-                    nMinOrderPos = wtx->nOrderPos;
+                        nMinOrderPos = wtx->nOrderPos;
-                    copyFrom = wtx;
+                        copyFrom = wtx;
+                    }
                 }
             }
             if (!copyFrom) {
@@ -627,7 +626,9 @@ void CWallet::SyncMetaData(const COutPoint& outPoint)
 
             // Now copy data from copyFrom to rest:
             for (auto &hash: hitN->second) {
-                CWalletTx *copyTo = &mapWallet.at(hash);
+                auto it = mapWallet.find(hash);
+                if (it != mapWallet.end()) continue;
+                CWalletTx* copyTo = &it->second;
                 if (copyFrom == copyTo) continue;
                 assert(copyFrom && "Oldest wallet transaction in range assumed to have been found.");
                 if (!copyFrom->IsEquivalentTo(*copyTo)) continue;
@@ -677,16 +678,13 @@ void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid)
 }
 
 
-void CWallet::AddToSpends(const uint256& wtxid)
+void CWallet::AddToSpends(const CWalletTx& wtx)
 {
-    auto it = mapWallet.find(wtxid);
+    if (wtx.IsCoinBase()) // Coinbases don't spend anything!
-    assert(it != mapWallet.end());
-    CWalletTx& thisTx = it->second;
-    if (thisTx.IsCoinBase()) // Coinbases don't spend anything!
         return;
 
-    for (const CTxIn& txin : thisTx.tx->vin)
+    for (const CTxIn& txin : wtx.tx->vin)
-        AddToSpends(txin.prevout, wtxid);
+        AddToSpends(txin.prevout, wtx.GetHash());
 }
 
 bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
@@ -988,7 +986,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 
     WalletBatch batch(*database, "r+", fFlushOnClose);
 
-    uint256 hash = wtxIn.GetHash();
+    const uint256& hash = wtxIn.GetHash();
 
     // Inserts only if not already there, returns tx inserted or tx found
     auto ret = mapWallet.emplace(hash, wtxIn);
@@ -1000,7 +998,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
         wtx.nOrderPos = IncOrderPosNext(&batch);
         wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, TxPair(&wtx, nullptr)));
         wtx.nTimeSmart = ComputeTimeSmart(wtx);
-        AddToSpends(hash);
+        AddToSpends(wtx);
     }
 
     bool fUpdated = false;
@@ -1069,14 +1067,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 
 void CWallet::LoadToWallet(const CWalletTx& wtxIn)
 {
-    uint256 hash = wtxIn.GetHash();
+    const uint256& hash = wtxIn.GetHash();
     const auto& ins = mapWallet.emplace(hash, wtxIn);
     CWalletTx& wtx = ins.first->second;
     wtx.BindWallet(this);
     if (/* insertion took place */ ins.second) {
         wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, TxPair(&wtx, nullptr)));
     }
-    AddToSpends(hash);
+    AddToSpends(wtx);
     for (const CTxIn& txin : wtx.tx->vin) {
         auto it = mapWallet.find(txin.prevout.hash);
         if (it != mapWallet.end()) {
@@ -1347,24 +1345,23 @@ void CWallet::BlockUntilSyncedToCurrentChain() {
     AssertLockNotHeld(cs_main);
     AssertLockNotHeld(cs_wallet);
 
-    {
+    // we want to ensure that head block is processed
-        // Skip the queue-draining stuff if we know we're caught up with
+    // otherwise it can lead to race condition or data misplace
-        // chainActive.Tip()...
+    while(true) {
-        // We could also take cs_wallet here, and call m_last_block_processed
+        {
-        // protected by cs_wallet instead of cs_main, but as long as we need
+            LOCK2(cs_main, cs_wallet);
-        // cs_main here anyway, it's easier to just call it cs_main-protected.
+            const CBlockIndex* initialChainTip = chainActive.Tip();
-        LOCK(cs_main);
-        const CBlockIndex* initialChainTip = chainActive.Tip();
 
-        if (m_last_block_processed && m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) {
+            if (m_last_block_processed && m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) {
-            return;
+                return;
+            }
         }
-    }
 
-    // ...otherwise put a callback in the validation interface queue and wait
+        // ...otherwise put a callback in the validation interface queue and wait
-    // for the queue to drain enough to execute it (indicating we are caught up
+        // for the queue to drain enough to execute it (indicating we are caught up
-    // at least with the time we entered this function).
+        // at least with the time we entered this function).
-    SyncWithValidationInterfaceQueue();
+        SyncWithValidationInterfaceQueue();
+    }
 }
 
 
@@ -1917,7 +1914,7 @@ std::set<uint256> CWalletTx::GetConflicts() const
     std::set<uint256> result;
     if (pwallet != nullptr)
     {
-        uint256 myHash = GetHash();
+        const uint256& myHash = GetHash();
         result = pwallet->GetConflicts(myHash);
         result.erase(myHash);
     }
@@ -2027,7 +2024,7 @@ CAmount CWalletTx::GetAvailableCredit(bool fUseCache, const isminefilter& filter
     }
 
     CAmount nCredit = 0;
-    uint256 hashTx = GetHash();
+    const uint256& hashTx = GetHash();
     for (unsigned int i = 0; i < tx->vout.size(); i++)
     {
         if (!pwallet->IsSpent(hashTx, i))
@@ -3257,9 +3254,18 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
 {
     AssertLockHeld(cs_wallet); // mapWallet
     DBErrors nZapSelectTxRet = WalletBatch(*database,"cr+").ZapSelectTx(vHashIn, vHashOut);
-    for (uint256 hash : vHashOut) {
+    for (const uint256& hash : vHashOut) {
-        const auto& it = mapWallet.find(hash);
+        auto it = mapWallet.find(hash);
+        if (it == mapWallet.end())
+            continue;
         wtxOrdered.erase(it->second.m_it_wtxOrdered);
+        for (auto& txin : it->second.tx->vin) {
+            auto mit = mapTxSpends.find(txin.prevout.hash);
+            if (mit != mapTxSpends.end()) {
+                if (mit->second.erase(txin.prevout.n) && mit->second.empty())
+                    mapTxSpends.erase(mit);
+            }
+        }
         mapWallet.erase(it);
     }
 
@@ -3849,7 +3855,7 @@ void CWallet::UnlockAllCoins()
     setLockedCoins.clear();
 }
 
-bool CWallet::IsLockedCoin(uint256 hash, unsigned int n) const
+bool CWallet::IsLockedCoin(const uint256& hash, unsigned int n) const
 {
     AssertLockHeld(cs_wallet); // setLockedCoins
     COutPoint outpt(hash, n);
@@ -4400,7 +4406,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const WalletLocation& loc
 
             for (const CWalletTx& wtxOld : vWtx)
             {
-                uint256 hash = wtxOld.GetHash();
+                const uint256& hash = wtxOld.GetHash();
                 auto mi = walletInstance->mapWallet.find(hash);
                 if (mi != walletInstance->mapWallet.end())
                 {

src/wallet/wallet.h

@@ -702,7 +702,7 @@ private:
     typedef robin_hood::unordered_map<uint256, robin_hood::unordered_map<uint32_t, std::vector<uint256>>> TxSpends;
     TxSpends mapTxSpends;
     void AddToSpends(const COutPoint& outpoint, const uint256& wtxid);
-    void AddToSpends(const uint256& wtxid);
+    void AddToSpends(const CWalletTx& wtx);
 
     /**
      * Add a transaction to the wallet, or update it.  pIndex and posInBlock should
@@ -877,7 +877,7 @@ public:
     bool IsSpent(const uint256& hash, unsigned int n) const;
     std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, bool single_coin) const;
 
-    bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool IsLockedCoin(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
@@ -1214,8 +1214,8 @@ public:
 
     /** Returns a bracketed wallet name for displaying in logs, will return [default wallet] if the wallet has no name */
     const std::string GetDisplayName() const {
-        std::string wallet_name = GetName().length() == 0 ? "default wallet" : GetName();
+        const std::string wallet_name = GetName();
-        return strprintf("[%s]", wallet_name);
+        return strprintf("[%s]", wallet_name.empty() ? "default wallet" : wallet_name);
     };
 
     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */

src/wallet/walletdb.cpp

@@ -51,7 +51,7 @@ bool WalletBatch::WriteTx(const CWalletTx& wtx)
     return WriteIC(std::make_pair(std::string("tx"), wtx.GetHash()), wtx);
 }
 
-bool WalletBatch::EraseTx(uint256 hash)
+bool WalletBatch::EraseTx(const uint256& hash)
 {
     return EraseIC(std::make_pair(std::string("tx"), hash));
 }
@@ -639,7 +639,7 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::vector<CW
 
     try {
         int nMinVersion = 0;
-        if (m_batch.Read((std::string)"minversion", nMinVersion))
+        if (m_batch.Read(std::string("minversion"), nMinVersion))
         {
             if (nMinVersion > FEATURE_LATEST)
                 return DBErrors::TOO_NEW;
@@ -707,10 +707,10 @@ DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<u
 
     // erase each matching wallet TX
     bool delerror = false;
-    std::vector<uint256>::iterator it = vTxHashIn.begin();
+    auto it = vTxHashIn.begin();
-    for (uint256 hash : vTxHash) {
+    for (const uint256& hash : vTxHash) {
-        while (it < vTxHashIn.end() && (*it) < hash) {
+        while (it != vTxHashIn.end() && (*it) < hash) {
-            it++;
+            ++it;
         }
         if (it == vTxHashIn.end()) {
             break;
@@ -739,7 +739,7 @@ DBErrors WalletBatch::ZapWalletTx(std::vector<CWalletTx>& vWtx)
         return err;
 
     // erase each wallet TX
-    for (uint256& hash : vTxHash) {
+    for (const uint256& hash : vTxHash) {
         if (!EraseTx(hash))
             return DBErrors::CORRUPT;
     }

src/wallet/walletdb.h

@@ -177,7 +177,7 @@ public:
     bool ErasePurpose(const std::string& strAddress);
 
     bool WriteTx(const CWalletTx& wtx);
-    bool EraseTx(uint256 hash);
+    bool EraseTx(const uint256& hash);
 
     bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata &keyMeta);
     bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector<unsigned char>& vchCryptedSecret, const CKeyMetadata &keyMeta);