From 356f254b62c02d66be5ae0295fcfa33c6c2205bd Mon Sep 17 00:00:00 2001 From: Anthony Fieroni Date: Wed, 4 Mar 2020 17:52:04 +0200 Subject: [PATCH] Fix wallet races and crashes Signed-off-by: Anthony Fieroni --- src/wallet/test/claim_rpc_tests.cpp | 21 +++++++ src/wallet/wallet.cpp | 88 +++++++++++++++-------------- src/wallet/wallet.h | 8 +-- src/wallet/walletdb.cpp | 14 ++--- src/wallet/walletdb.h | 2 +- 5 files changed, 80 insertions(+), 53 deletions(-) diff --git a/src/wallet/test/claim_rpc_tests.cpp b/src/wallet/test/claim_rpc_tests.cpp index 49ef481b1..a016a0253 100644 --- a/src/wallet/test/claim_rpc_tests.cpp +++ b/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 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() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c6b46787e..208760d36 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -44,7 +44,7 @@ bool AddWallet(const std::shared_ptr& wallet) { LOCK(cs_wallets); assert(wallet); - std::vector>::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 CWallet::GetConflicts(const uint256& txid) const return result; const CWalletTx& wtx = it->second; - std::pair 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 (wtx->nOrderPos < nMinOrderPos) { - nMinOrderPos = wtx->nOrderPos; - copyFrom = wtx; + if (auto wtx = GetWalletTx(spend)) { + if (wtx->nOrderPos < nMinOrderPos) { + nMinOrderPos = wtx->nOrderPos; + 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); - assert(it != mapWallet.end()); - CWalletTx& thisTx = it->second; - if (thisTx.IsCoinBase()) // Coinbases don't spend anything! + if (wtx.IsCoinBase()) // Coinbases don't spend anything! return; - for (const CTxIn& txin : thisTx.tx->vin) - AddToSpends(txin.prevout, wtxid); + for (const CTxIn& txin : wtx.tx->vin) + 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); - { - // Skip the queue-draining stuff if we know we're caught up with - // chainActive.Tip()... - // 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 - // cs_main here anyway, it's easier to just call it cs_main-protected. - LOCK(cs_main); - const CBlockIndex* initialChainTip = chainActive.Tip(); + // we want to ensure that head block is processed + // otherwise it can lead to race condition or data misplace + while(true) { + { + LOCK2(cs_main, cs_wallet); + const CBlockIndex* initialChainTip = chainActive.Tip(); - if (m_last_block_processed && m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) { - return; + if (m_last_block_processed && m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) { + return; + } } - } - // ...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 - // at least with the time we entered this function). - SyncWithValidationInterfaceQueue(); + // ...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 + // at least with the time we entered this function). + SyncWithValidationInterfaceQueue(); + } } @@ -1917,7 +1914,7 @@ std::set CWalletTx::GetConflicts() const std::set 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& vHashIn, std::vectorsecond.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::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()) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 48addbc3b..5ca3d8189 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -702,7 +702,7 @@ private: typedef robin_hood::unordered_map>> 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 GroupOutputs(const std::vector& 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(); - return strprintf("[%s]", wallet_name); + const std::string wallet_name = GetName(); + 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 */ diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 15c5b82c5..6ac75262b 100644 --- a/src/wallet/walletdb.cpp +++ b/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& vTxHash, std::vector FEATURE_LATEST) return DBErrors::TOO_NEW; @@ -707,10 +707,10 @@ DBErrors WalletBatch::ZapSelectTx(std::vector& vTxHashIn, std::vector::iterator it = vTxHashIn.begin(); - for (uint256 hash : vTxHash) { - while (it < vTxHashIn.end() && (*it) < hash) { - it++; + auto it = vTxHashIn.begin(); + for (const uint256& hash : vTxHash) { + while (it != vTxHashIn.end() && (*it) < hash) { + ++it; } if (it == vTxHashIn.end()) { break; @@ -739,7 +739,7 @@ DBErrors WalletBatch::ZapWalletTx(std::vector& vWtx) return err; // erase each wallet TX - for (uint256& hash : vTxHash) { + for (const uint256& hash : vTxHash) { if (!EraseTx(hash)) return DBErrors::CORRUPT; } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 0e6cb1bba..df272b6fa 100644 --- a/src/wallet/walletdb.h +++ b/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& vchCryptedSecret, const CKeyMetadata &keyMeta);