From ca3655f494f073aa35e60fecaf8c8a233f41150f Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 9 Mar 2018 17:15:48 +0300 Subject: [PATCH] Fix some (potential dead)locks (#1977) * Avoid locking cs_main in CMasternode/Ping/Broadcast Deligate this to CMasternodeMan and use AssertLockHeld instead * Ensure consistent locking order (cs_main, cs_wallet, mempool.cs, cs_instantsend) in CInstantSend while avoiding potential deadlocks at the same time * Add missing locks in wallet * Add missing locks in governance * Fix cs_vPendingMasternodes vs cs_main potential deadlock SendVerifyRequest no longer opens connection directly, so no cs_main is needed here --- src/governance-object.cpp | 14 ++++++- src/governance.cpp | 2 + src/instantx.cpp | 83 ++++++++++++++++++++------------------- src/masternode.cpp | 11 ++++-- src/masternodeman.cpp | 8 ++-- src/wallet/wallet.cpp | 6 +++ 6 files changed, 74 insertions(+), 50 deletions(-) diff --git a/src/governance-object.cpp b/src/governance-object.cpp index e931d7ed94..eadc3491d2 100644 --- a/src/governance-object.cpp +++ b/src/governance-object.cpp @@ -102,6 +102,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom, CConnman& connman) { if(!mnodeman.Has(vote.GetMasternodeOutpoint())) { + LOCK(cs); std::ostringstream ostr; ostr << "CGovernanceObject::ProcessVote -- Masternode " << vote.GetMasternodeOutpoint().ToStringShort() << " not found"; exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_WARNING); @@ -117,6 +118,9 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom, return false; } + vote_instance_t voteInstance; + { + LOCK(cs); vote_m_it it = mapCurrentMNVotes.find(vote.GetMasternodeOutpoint()); if(it == mapCurrentMNVotes.end()) { it = mapCurrentMNVotes.insert(vote_m_t::value_type(vote.GetMasternodeOutpoint(), vote_rec_t())).first; @@ -141,7 +145,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom, if(it2 == recVote.mapInstances.end()) { it2 = recVote.mapInstances.insert(vote_instance_m_t::value_type(int(eSignal), vote_instance_t())).first; } - vote_instance_t& voteInstance = it2->second; + voteInstance = it2->second; // Reject obsolete votes if(vote.GetTimestamp() < voteInstance.nCreationTime) { @@ -151,6 +155,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom, exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_NONE); return false; } + } // LOCK(cs) int64_t nNow = GetAdjustedTime(); int64_t nVoteTimeUpdate = voteInstance.nTime; @@ -189,6 +194,7 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom, exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_PERMANENT_ERROR); return false; } + LOCK(cs); voteInstance = vote_instance_t(vote.GetOutcome(), nVoteTimeUpdate, vote.GetTimestamp()); if(!fileVotes.HasVote(vote.GetHash())) { fileVotes.AddVote(vote); @@ -199,6 +205,8 @@ bool CGovernanceObject::ProcessVote(CNode* pfrom, void CGovernanceObject::ClearMasternodeVotes() { + LOCK(cs); + vote_m_it it = mapCurrentMNVotes.begin(); while(it != mapCurrentMNVotes.end()) { if(!mnodeman.Has(it->first)) { @@ -628,6 +636,8 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC int CGovernanceObject::CountMatchingVotes(vote_signal_enum_t eVoteSignalIn, vote_outcome_enum_t eVoteOutcomeIn) const { + LOCK(cs); + int nCount = 0; for(vote_m_cit it = mapCurrentMNVotes.begin(); it != mapCurrentMNVotes.end(); ++it) { const vote_rec_t& recVote = it->second; @@ -674,6 +684,8 @@ int CGovernanceObject::GetAbstainCount(vote_signal_enum_t eVoteSignalIn) const bool CGovernanceObject::GetCurrentMNVotes(const COutPoint& mnCollateralOutpoint, vote_rec_t& voteRecord) { + LOCK(cs); + vote_m_it it = mapCurrentMNVotes.find(mnCollateralOutpoint); if (it == mapCurrentMNVotes.end()) { return false; diff --git a/src/governance.cpp b/src/governance.cpp index d8b91ce7fe..5cd2632ed4 100644 --- a/src/governance.cpp +++ b/src/governance.cpp @@ -1164,6 +1164,8 @@ bool CGovernanceManager::AcceptMessage(const uint256& nHash, hash_s_t& setHash) void CGovernanceManager::RebuildIndexes() { + LOCK(cs); + cmapVoteToObject.Clear(); for(object_m_it it = mapObjects.begin(); it != mapObjects.end(); ++it) { CGovernanceObject& govobj = it->second; diff --git a/src/instantx.cpp b/src/instantx.cpp index e7b22b02c2..2fb093f9da 100644 --- a/src/instantx.cpp +++ b/src/instantx.cpp @@ -69,15 +69,11 @@ void CInstantSend::ProcessMessage(CNode* pfrom, const std::string& strCommand, C // Ignore any InstantSend messages until masternode list is synced if(!masternodeSync.IsMasternodeListSynced()) return; - LOCK(cs_main); -#ifdef ENABLE_WALLET - if (pwalletMain) - LOCK(pwalletMain->cs_wallet); -#endif - LOCK(cs_instantsend); - - if(mapTxLockVotes.count(nVoteHash)) return; - mapTxLockVotes.insert(std::make_pair(nVoteHash, vote)); + { + LOCK(cs_instantsend); + auto ret = mapTxLockVotes.emplace(nVoteHash, vote); + if (!ret.second) return; + } ProcessNewTxLockVote(pfrom, vote, connman); @@ -87,7 +83,11 @@ void CInstantSend::ProcessMessage(CNode* pfrom, const std::string& strCommand, C bool CInstantSend::ProcessTxLockRequest(const CTxLockRequest& txLockRequest, CConnman& connman) { - LOCK2(cs_main, cs_instantsend); + LOCK(cs_main); +#ifdef ENABLE_WALLET + LOCK(pwalletMain ? &pwalletMain->cs_wallet : NULL); +#endif + LOCK2(mempool.cs, cs_instantsend); uint256 txHash = txLockRequest.GetHash(); @@ -183,13 +183,23 @@ void CInstantSend::CreateEmptyTxLockCandidate(const uint256& txHash) void CInstantSend::Vote(const uint256& txHash, CConnman& connman) { AssertLockHeld(cs_main); - LOCK(cs_instantsend); +#ifdef ENABLE_WALLET + LOCK(pwalletMain ? &pwalletMain->cs_wallet : NULL); +#endif + + CTxLockRequest dummyRequest; + CTxLockCandidate txLockCandidate(dummyRequest); + { + LOCK(cs_instantsend); + auto itLockCandidate = mapTxLockCandidates.find(txHash); + if (itLockCandidate == mapTxLockCandidates.end()) return; + txLockCandidate = itLockCandidate->second; + Vote(txLockCandidate, connman); + } - std::map::iterator itLockCandidate = mapTxLockCandidates.find(txHash); - if (itLockCandidate == mapTxLockCandidates.end()) return; - Vote(itLockCandidate->second, connman); // Let's see if our vote changed smth - TryToFinalizeLockCandidate(itLockCandidate->second); + LOCK2(mempool.cs, cs_instantsend); + TryToFinalizeLockCandidate(txLockCandidate); } void CInstantSend::Vote(CTxLockCandidate& txLockCandidate, CConnman& connman) @@ -197,7 +207,8 @@ void CInstantSend::Vote(CTxLockCandidate& txLockCandidate, CConnman& connman) if(!fMasternodeMode) return; if(!sporkManager.IsSporkActive(SPORK_2_INSTANTSEND_ENABLED)) return; - LOCK2(cs_main, cs_instantsend); + AssertLockHeld(cs_main); + AssertLockHeld(cs_instantsend); uint256 txHash = txLockCandidate.GetHash(); // We should never vote on a Transaction Lock Request that was not (yet) accepted by the mempool @@ -295,14 +306,6 @@ void CInstantSend::Vote(CTxLockCandidate& txLockCandidate, CConnman& connman) bool CInstantSend::ProcessNewTxLockVote(CNode* pfrom, const CTxLockVote& vote, CConnman& connman) { - // cs_main, cs_wallet and cs_instantsend should be already locked - AssertLockHeld(cs_main); -#ifdef ENABLE_WALLET - if (pwalletMain) - AssertLockHeld(pwalletMain->cs_wallet); -#endif - AssertLockHeld(cs_instantsend); - uint256 txHash = vote.GetTxHash(); uint256 nVoteHash = vote.GetHash(); @@ -315,6 +318,12 @@ bool CInstantSend::ProcessNewTxLockVote(CNode* pfrom, const CTxLockVote& vote, C // relay valid vote asap vote.Relay(connman); + LOCK(cs_main); +#ifdef ENABLE_WALLET + LOCK(pwalletMain ? &pwalletMain->cs_wallet : NULL); +#endif + LOCK2(mempool.cs, cs_instantsend); + // Masternodes will sometimes propagate votes before the transaction is known to the client, // will actually process only after the lock request itself has arrived @@ -457,12 +466,8 @@ void CInstantSend::UpdateVotedOutpoints(const CTxLockVote& vote, CTxLockCandidat void CInstantSend::ProcessOrphanTxLockVotes() { - LOCK(cs_main); -#ifdef ENABLE_WALLET - if (pwalletMain) - LOCK(pwalletMain->cs_wallet); -#endif - LOCK(cs_instantsend); + AssertLockHeld(cs_main); + AssertLockHeld(cs_instantsend); std::map::iterator it = mapTxLockVotesOrphan.begin(); while(it != mapTxLockVotesOrphan.end()) { @@ -496,12 +501,8 @@ void CInstantSend::TryToFinalizeLockCandidate(const CTxLockCandidate& txLockCand { if(!sporkManager.IsSporkActive(SPORK_2_INSTANTSEND_ENABLED)) return; - LOCK(cs_main); -#ifdef ENABLE_WALLET - if (pwalletMain) - LOCK(pwalletMain->cs_wallet); -#endif - LOCK(cs_instantsend); + AssertLockHeld(cs_main); + AssertLockHeld(cs_instantsend); uint256 txHash = txLockCandidate.txLockRequest.tx->GetHash(); if(txLockCandidate.IsAllOutPointsReady() && !IsLockedInstantSendTransaction(txHash)) { @@ -516,7 +517,8 @@ void CInstantSend::TryToFinalizeLockCandidate(const CTxLockCandidate& txLockCand void CInstantSend::UpdateLockedTransaction(const CTxLockCandidate& txLockCandidate) { - // cs_wallet and cs_instantsend should be already locked + // cs_main, cs_wallet and cs_instantsend should be already locked + AssertLockHeld(cs_main); #ifdef ENABLE_WALLET if (pwalletMain) AssertLockHeld(pwalletMain->cs_wallet); @@ -575,14 +577,15 @@ bool CInstantSend::GetLockedOutPointTxHash(const COutPoint& outpoint, uint256& h bool CInstantSend::ResolveConflicts(const CTxLockCandidate& txLockCandidate) { - LOCK2(cs_main, cs_instantsend); + AssertLockHeld(cs_main); + AssertLockHeld(cs_instantsend); uint256 txHash = txLockCandidate.GetHash(); // make sure the lock is ready if(!txLockCandidate.IsAllOutPointsReady()) return false; - LOCK(mempool.cs); // protect mempool.mapNextTx + AssertLockHeld(mempool.cs); // protect mempool.mapNextTx for (const auto& txin : txLockCandidate.txLockRequest.tx->vin) { uint256 hashConflicting; @@ -953,7 +956,7 @@ bool CTxLockRequest::IsValid() const LogPrint("instantsend", "CTxLockRequest::IsValid -- WARNING: Too many inputs: tx=%s", ToString()); } - LOCK(cs_main); + AssertLockHeld(cs_main); if(!CheckFinalTx(*tx)) { LogPrint("instantsend", "CTxLockRequest::IsValid -- Transaction is not final: tx=%s", ToString()); return false; diff --git a/src/masternode.cpp b/src/masternode.cpp index 961ae7fc18..2f2924b9d0 100644 --- a/src/masternode.cpp +++ b/src/masternode.cpp @@ -130,6 +130,7 @@ CMasternode::CollateralStatus CMasternode::CheckCollateral(const COutPoint& outp void CMasternode::Check(bool fForce) { + AssertLockHeld(cs_main); LOCK(cs); if(ShutdownRequested()) return; @@ -144,9 +145,6 @@ void CMasternode::Check(bool fForce) int nHeight = 0; if(!fUnitTest) { - TRY_LOCK(cs_main, lockMain); - if(!lockMain) return; - Coin coin; if(!GetUTXOCoin(outpoint, coin)) { nActiveState = MASTERNODE_OUTPOINT_SPENT; @@ -415,6 +413,8 @@ bool CMasternodeBroadcast::SimpleCheck(int& nDos) { nDos = 0; + AssertLockHeld(cs_main); + // make sure addr is valid if(!IsValidNetAddr()) { LogPrintf("CMasternodeBroadcast::SimpleCheck -- Invalid addr, rejected: masternode=%s addr=%s\n", @@ -470,6 +470,8 @@ bool CMasternodeBroadcast::Update(CMasternode* pmn, int& nDos, CConnman& connman { nDos = 0; + AssertLockHeld(cs_main); + if(pmn->sigTime == sigTime && !fRecovery) { // mapSeenMasternodeBroadcast in CMasternodeMan::CheckMnbAndUpdateMasternodeList should filter legit duplicates // but this still can happen if we just started, which is ok, just do nothing here. @@ -820,6 +822,8 @@ bool CMasternodePing::SimpleCheck(int& nDos) bool CMasternodePing::CheckAndUpdate(CMasternode* pmn, bool fFromNewBroadcast, int& nDos, CConnman& connman) { + AssertLockHeld(cs_main); + // don't ban by default nDos = 0; @@ -845,7 +849,6 @@ bool CMasternodePing::CheckAndUpdate(CMasternode* pmn, bool fFromNewBroadcast, i } { - LOCK(cs_main); BlockMap::iterator mi = mapBlockIndex.find(blockHash); if ((*mi).second && (*mi).second->nHeight < chainActive.Height() - 24) { LogPrintf("CMasternodePing::CheckAndUpdate -- Masternode ping is invalid, block hash is too old: masternode=%s blockHash=%s\n", masternodeOutpoint.ToStringShort(), blockHash.ToString()); diff --git a/src/masternodeman.cpp b/src/masternodeman.cpp index a31f22f8aa..b8af7862b9 100644 --- a/src/masternodeman.cpp +++ b/src/masternodeman.cpp @@ -156,7 +156,7 @@ bool CMasternodeMan::PoSeBan(const COutPoint &outpoint) void CMasternodeMan::Check() { - LOCK(cs); + LOCK2(cs_main, cs); LogPrint("masternode", "CMasternodeMan::Check -- nLastSentinelPingTime=%d, IsSentinelPingActive()=%d\n", nLastSentinelPingTime, IsSentinelPingActive()); @@ -999,9 +999,7 @@ void CMasternodeMan::DoFullVerificationStep(CConnman& connman) rank_pair_vec_t vecMasternodeRanks; GetMasternodeRanks(vecMasternodeRanks, nCachedBlockHeight - 1, MIN_POSE_PROTO_VERSION); - // Need LOCK2 here to ensure consistent locking order because the SendVerifyRequest call below locks cs_main - // through InitializeNode signal in OpenNetworkConnection - LOCK2(cs_main, cs); + LOCK(cs); int nCount = 0; @@ -1643,7 +1641,7 @@ void CMasternodeMan::RemoveGovernanceObject(uint256 nGovernanceObjectHash) void CMasternodeMan::CheckMasternode(const CPubKey& pubKeyMasternode, bool fForce) { - LOCK(cs); + LOCK2(cs_main, cs); for (auto& mnpair : mapMasternodes) { if (mnpair.second.pubKeyMasternode == pubKeyMasternode) { mnpair.second.Check(fForce); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8fd9212598..ffb4bf4095 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3193,6 +3193,8 @@ bool CWallet::SelectCoinsDark(CAmount nValueMin, CAmount nValueMax, std::vector< bool CWallet::GetCollateralTxDSIn(CTxDSIn& txdsinRet, CAmount& nValueRet) const { + LOCK2(cs_main, cs_wallet); + std::vector vCoins; AvailableCoins(vCoins); @@ -3305,6 +3307,8 @@ bool CWallet::HasCollateralInputs(bool fOnlyConfirmed) const bool CWallet::CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason) { + LOCK2(cs_main, cs_wallet); + txCollateral.vin.clear(); txCollateral.vout.clear(); @@ -3375,6 +3379,8 @@ bool CWallet::GetBudgetSystemCollateralTX(CWalletTx& tx, uint256 hash, CAmount a bool CWallet::ConvertList(std::vector vecTxIn, std::vector& vecAmounts) { + LOCK2(cs_main, cs_wallet); + for (const auto& txin : vecTxIn) { if (mapWallet.count(txin.prevout.hash)) { CWalletTx& wtx = mapWallet[txin.prevout.hash];