Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deadlocks #1168

Closed
wants to merge 3 commits into from
Closed

Deadlocks #1168

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de00f7e
fix locks
UdjinM6 Nov 25, 2016
6a8b153
centralize CMasternode[Man]::Check() calls, remove related cs_main locks
UdjinM6 Nov 25, 2016
1975324
revert CGovernanceManager::ConfirmInventoryRequest lock removal
UdjinM6 Nov 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 46 additions & 26 deletions src/darksend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ void CDarksendPool::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataS
}

} else if(strCommand == NetMsgType::DSQUEUE) {
TRY_LOCK(cs_darksend, lockRecv);
if(!lockRecv) return;

if(pfrom->nVersion < MIN_PRIVATESEND_PEER_PROTO_VERSION) {
LogPrint("privatesend", "DSQUEUE -- incompatible version! nVersion: %d\n", pfrom->nVersion);
Expand All @@ -101,11 +99,14 @@ void CDarksendPool::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataS
CDarksendQueue dsq;
vRecv >> dsq;

// process every dsq only once
BOOST_FOREACH(CDarksendQueue q, vecDarksendQueue) {
if(q == dsq) {
// LogPrint("privatesend", "DSQUEUE -- %s seen\n", dsq.ToString());
return;
{
LOCK(cs_darksend);
// process every dsq only once
BOOST_FOREACH(CDarksendQueue q, vecDarksendQueue) {
if(q == dsq) {
// LogPrint("privatesend", "DSQUEUE -- %s seen\n", dsq.ToString());
return;
}
}
}

Expand Down Expand Up @@ -135,11 +136,15 @@ void CDarksendPool::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataS
SubmitDenominate();
}
} else {
BOOST_FOREACH(CDarksendQueue q, vecDarksendQueue) {
if(q.vin == dsq.vin) {
// no way same mn can send another "not yet ready" dsq this soon
LogPrint("privatesend", "DSQUEUE -- Masternode %s is sending WAY too many dsq messages\n", pmn->addr.ToString());
return;

{
LOCK(cs_darksend);
BOOST_FOREACH(CDarksendQueue q, vecDarksendQueue) {
if(q.vin == dsq.vin) {
// no way same mn can send another "not yet ready" dsq this soon
LogPrint("privatesend", "DSQUEUE -- Masternode %s is sending WAY too many dsq messages\n", pmn->addr.ToString());
return;
}
}
}

Expand Down Expand Up @@ -724,6 +729,8 @@ void CDarksendPool::ChargeFees()
LogPrintf("CDarksendPool::ChargeFees -- found uncooperative node (didn't %s transaction), charging fees: %s\n",
(nState == POOL_STATE_SIGNING) ? "sign" : "send", vecOffendersCollaterals[0].ToString());

LOCK(cs_main);

CValidationState state;
bool fMissingInputs;
if(!AcceptToMemoryPool(mempool, state, vecOffendersCollaterals[0], false, &fMissingInputs, false, true)) {
Expand Down Expand Up @@ -751,6 +758,8 @@ void CDarksendPool::ChargeRandomFees()
{
if(!fMasterNode) return;

LOCK(cs_main);

BOOST_FOREACH(const CTransaction& txCollateral, vecSessionCollaterals) {

if(GetRandInt(100) > 10) return;
Expand All @@ -773,13 +782,17 @@ void CDarksendPool::ChargeRandomFees()
//
void CDarksendPool::CheckTimeout()
{
// check mixing queue objects for timeouts
std::vector<CDarksendQueue>::iterator it = vecDarksendQueue.begin();
while(it != vecDarksendQueue.end()) {
if((*it).IsExpired()) {
LogPrint("privatesend", "CDarksendPool::CheckTimeout -- Removing expired queue (%s)\n", (*it).ToString());
it = vecDarksendQueue.erase(it);
} else ++it;
{
LOCK(cs_darksend);

// check mixing queue objects for timeouts
std::vector<CDarksendQueue>::iterator it = vecDarksendQueue.begin();
while(it != vecDarksendQueue.end()) {
if((*it).IsExpired()) {
LogPrint("privatesend", "CDarksendPool::CheckTimeout -- Removing expired queue (%s)\n", (*it).ToString());
it = vecDarksendQueue.erase(it);
} else ++it;
}
}

if(!fEnablePrivateSend && !fMasterNode) return;
Expand Down Expand Up @@ -1347,11 +1360,7 @@ bool CDarksendPool::DoAutomaticDenominating(bool fDryRun)
return false;
}

TRY_LOCK(cs_darksend, lockDS);
if(!lockDS) {
strAutoDenomResult = _("Lock is already in place.");
return false;
}
LOCK2(cs_main, cs_darksend);

if(!fDryRun && pwalletMain->IsLocked(true)) {
strAutoDenomResult = _("Wallet is locked.");
Expand Down Expand Up @@ -2335,11 +2344,22 @@ bool CDarksendQueue::CheckSignature(const CPubKey& pubKeyMasternode)

bool CDarksendQueue::Relay()
{
LOCK(cs_vNodes);
BOOST_FOREACH(CNode* pnode, vNodes)
std::vector<CNode*> vNodesCopy;
{
LOCK(cs_vNodes);
vNodesCopy = vNodes;
BOOST_FOREACH(CNode* pnode, vNodesCopy)
pnode->AddRef();
}
BOOST_FOREACH(CNode* pnode, vNodesCopy)
if(pnode->nVersion >= MIN_PRIVATESEND_PEER_PROTO_VERSION)
pnode->PushMessage(NetMsgType::DSQUEUE, (*this));

{
LOCK(cs_vNodes);
BOOST_FOREACH(CNode* pnode, vNodesCopy)
pnode->Release();
}
return true;
}

Expand Down
15 changes: 8 additions & 7 deletions src/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,6 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C

if(pfrom->nVersion < MIN_GOVERNANCE_PEER_PROTO_VERSION) return;

LOCK(cs);

// ANOTHER USER IS ASKING US TO HELP THEM SYNC GOVERNANCE OBJECT DATA
if (strCommand == NetMsgType::MNGOVERNANCESYNC)
{
Expand Down Expand Up @@ -164,6 +162,8 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C
return;
}

LOCK2(cs_main, cs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see CountEnabled() changed to not call Check() than adding all these locks. I think that is the only function we call from these governance functions that locks cs_main.

Same comment for the other governance functions and rpcgovernance.cpp


if(mapSeenGovernanceObjects.count(nHash)) {
// TODO - print error code? what if it's GOVOBJ_ERROR_IMMATURE?
LogPrint("gobject", "CGovernanceManager -- Received already seen object: %s\n", strHash);
Expand Down Expand Up @@ -278,7 +278,7 @@ void CGovernanceManager::CheckOrphanVotes(CNode* pfrom, CGovernanceObject& govob

bool CGovernanceManager::AddGovernanceObject(CGovernanceObject& govobj)
{
LOCK(cs);
LOCK2(cs_main, cs);
std::string strError = "";

DBG( cout << "CGovernanceManager::AddGovernanceObject START" << endl; );
Expand Down Expand Up @@ -333,7 +333,7 @@ void CGovernanceManager::UpdateCachesAndClean()

std::vector<uint256> vecDirtyHashes = mnodeman.GetAndClearDirtyGovernanceObjectHashes();

LOCK(cs);
LOCK2(cs_main, cs);

for(size_t i = 0; i < vecDirtyHashes.size(); ++i) {
object_m_it it = mapObjects.find(vecDirtyHashes[i]);
Expand Down Expand Up @@ -542,7 +542,8 @@ void CGovernanceManager::NewBlock()

bool CGovernanceManager::ConfirmInventoryRequest(const CInv& inv)
{
LOCK(cs);
// FIXME:
// LOCK(cs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the problem with this ?

This is called only from AlreadyHave() which is called from 2 places in main.cpp. I don't see any locks held there. Should be OK if cs_main is held on entry since that would be the correct order.


LogPrint("gobject", "CGovernanceManager::ConfirmInventoryRequest inv = %s\n", inv.ToString());

Expand Down Expand Up @@ -606,7 +607,7 @@ void CGovernanceManager::Sync(CNode* pfrom, uint256 nProp)
// SYNC GOVERNANCE OBJECTS WITH OTHER CLIENT

{
LOCK(cs);
LOCK2(cs_main, cs);
for(object_m_it it = mapObjects.begin(); it != mapObjects.end(); ++it) {
uint256 h = it->first;

Expand Down Expand Up @@ -867,7 +868,7 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex *pindex)
return;
}

LOCK(cs);
LOCK2(cs_main, cs);
pCurrentBlockIndex = pindex;
nCachedBlockHeight = pCurrentBlockIndex->nHeight;
LogPrint("gobject", "CGovernanceManager::UpdatedBlockTip pCurrentBlockIndex->nHeight: %d\n", pCurrentBlockIndex->nHeight);
Expand Down
17 changes: 10 additions & 7 deletions src/instantx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,12 @@ bool IsInstantSendTxValid(const CTransaction& txCandidate)
{
if(txCandidate.vout.size() < 1) return false;

if(!CheckFinalTx(txCandidate)) {
LogPrint("instantsend", "IsInstantSendTxValid -- Transaction is not final: txCandidate=%s", txCandidate.ToString());
return false;
{
LOCK(cs_main);
if(!CheckFinalTx(txCandidate)) {
LogPrint("instantsend", "IsInstantSendTxValid -- Transaction is not final: txCandidate=%s", txCandidate.ToString());
return false;
}
}

int64_t nValueIn = 0;
Expand Down Expand Up @@ -460,16 +463,16 @@ int64_t GetAverageMasternodeOrphanVoteTime()

void CleanTxLockCandidates()
{
LOCK(cs_instantsend);

std::map<uint256, CTxLockCandidate>::iterator it = mapTxLockCandidates.begin();

int nHeight;
{
LOCK(cs_main);
nHeight = chainActive.Height();
}

LOCK(cs_instantsend);

std::map<uint256, CTxLockCandidate>::iterator it = mapTxLockCandidates.begin();

while(it != mapTxLockCandidates.end()) {
CTxLockCandidate &txLockCandidate = it->second;
if(nHeight > txLockCandidate.nExpirationBlock) {
Expand Down
6 changes: 4 additions & 2 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5057,14 +5057,16 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
{
// Send stream from relay memory
bool pushed = false;
map<CInv, CDataStream>::iterator mi;
{
LOCK(cs_mapRelay);
map<CInv, CDataStream>::iterator mi = mapRelay.find(inv);
mi = mapRelay.find(inv);
if (mi != mapRelay.end()) {
pfrom->PushMessage(inv.GetCommand(), (*mi).second);
pushed = true;
}
}
if(pushed)
pfrom->PushMessage(inv.GetCommand(), (*mi).second);

if (!pushed && inv.type == MSG_TX) {
CTransaction tx;
Expand Down
6 changes: 3 additions & 3 deletions src/masternode-payments.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ bool CMasternodePayments::AddPaymentVote(const CMasternodePaymentVote& vote)
uint256 blockHash = uint256();
if(!GetBlockHash(blockHash, vote.nBlockHeight - 101)) return false;

LOCK2(cs_mapMasternodePaymentVotes, cs_mapMasternodeBlocks);
LOCK2(cs_mapMasternodeBlocks, cs_mapMasternodePaymentVotes);

if(mapMasternodePaymentVotes.count(vote.GetHash())) return false;

Expand Down Expand Up @@ -573,7 +573,7 @@ void CMasternodePayments::CheckAndRemove()
{
if(!pCurrentBlockIndex) return;

LOCK2(cs_mapMasternodePaymentVotes, cs_mapMasternodeBlocks);
LOCK2(cs_mapMasternodeBlocks, cs_mapMasternodePaymentVotes);

int nLimit = GetStorageLimit();

Expand Down Expand Up @@ -781,7 +781,7 @@ void CMasternodePayments::RequestLowDataPaymentBlocks(CNode* pnode)
// Old nodes can't process this
if(pnode->nVersion < 70202) return;

LOCK(cs_mapMasternodeBlocks);
LOCK2(cs_main, cs_mapMasternodeBlocks);

std::vector<CInv> vToFetch;
std::map<int, CMasternodeBlockPayees>::iterator it = mapMasternodeBlocks.begin();
Expand Down
30 changes: 27 additions & 3 deletions src/masternode-sync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,13 @@ void CMasternodeSync::ClearFulfilledRequests()
}
}

void ReleaseNodes(const std::vector<CNode*> &vNodesCopy)
{
LOCK(cs_vNodes);
BOOST_FOREACH(CNode* pnode, vNodesCopy)
pnode->Release();
}

void CMasternodeSync::ProcessTick()
{
static int nTick = 0;
Expand Down Expand Up @@ -210,15 +217,21 @@ void CMasternodeSync::ProcessTick()
return;
}

LOCK2(mnodeman.cs, cs_vNodes);

if(nRequestedMasternodeAssets == MASTERNODE_SYNC_INITIAL ||
(nRequestedMasternodeAssets == MASTERNODE_SYNC_SPORKS && IsBlockchainSynced()))
{
SwitchToNextAsset();
}

BOOST_FOREACH(CNode* pnode, vNodes)
std::vector<CNode*> vNodesCopy;
{
LOCK(cs_vNodes);
vNodesCopy = vNodes;
BOOST_FOREACH(CNode* pnode, vNodesCopy)
pnode->AddRef();
}

BOOST_FOREACH(CNode* pnode, vNodesCopy)
{
// QUICK MODE (REGTEST ONLY!)
if(Params().NetworkIDString() == CBaseChainParams::REGTEST)
Expand All @@ -236,6 +249,7 @@ void CMasternodeSync::ProcessTick()
nRequestedMasternodeAssets = MASTERNODE_SYNC_FINISHED;
}
nRequestedMasternodeAttempt++;
ReleaseNodes(vNodesCopy);
return;
}

Expand Down Expand Up @@ -271,9 +285,11 @@ void CMasternodeSync::ProcessTick()
LogPrintf("CMasternodeSync::ProcessTick -- ERROR: failed to sync %s\n", GetAssetName());
// there is no way we can continue without masternode list, fail here and try later
Fail();
ReleaseNodes(vNodesCopy);
return;
}
SwitchToNextAsset();
ReleaseNodes(vNodesCopy);
return;
}

Expand All @@ -288,6 +304,7 @@ void CMasternodeSync::ProcessTick()
if(nRequestedMasternodeAttempt > 1 && nMnCount > nMnCountEstimated) {
LogPrintf("CMasternodeSync::ProcessTick -- nTick %d nRequestedMasternodeAssets %d -- found enough data\n", nTick, nRequestedMasternodeAssets);
SwitchToNextAsset();
ReleaseNodes(vNodesCopy);
return;
}

Expand All @@ -300,6 +317,7 @@ void CMasternodeSync::ProcessTick()

mnodeman.DsegUpdate(pnode);

ReleaseNodes(vNodesCopy);
return; //this will cause each peer to get one request each six seconds for the various assets we need
}

Expand All @@ -316,9 +334,11 @@ void CMasternodeSync::ProcessTick()
LogPrintf("CMasternodeSync::ProcessTick -- ERROR: failed to sync %s\n", GetAssetName());
// probably not a good idea to proceed without winner list
Fail();
ReleaseNodes(vNodesCopy);
return;
}
SwitchToNextAsset();
ReleaseNodes(vNodesCopy);
return;
}

Expand All @@ -328,6 +348,7 @@ void CMasternodeSync::ProcessTick()
if(nRequestedMasternodeAttempt > 1 && mnpayments.IsEnoughData()) {
LogPrintf("CMasternodeSync::ProcessTick -- nTick %d nRequestedMasternodeAssets %d -- found enough data\n", nTick, nRequestedMasternodeAssets);
SwitchToNextAsset();
ReleaseNodes(vNodesCopy);
return;
}

Expand All @@ -343,6 +364,7 @@ void CMasternodeSync::ProcessTick()
// ask node for missing pieces only (old nodes will not be asked)
mnpayments.RequestLowDataPaymentBlocks(pnode);

ReleaseNodes(vNodesCopy);
return; //this will cause each peer to get one request each six seconds for the various assets we need
}

Expand All @@ -359,6 +381,7 @@ void CMasternodeSync::ProcessTick()
// it's kind of ok to skip this for now, hopefully we'll catch up later?
}
SwitchToNextAsset();
ReleaseNodes(vNodesCopy);
return;
}

Expand All @@ -384,6 +407,7 @@ void CMasternodeSync::ProcessTick()

pnode->PushMessage(NetMsgType::MNGOVERNANCESYNC, uint256()); //sync masternode votes

ReleaseNodes(vNodesCopy);
return; //this will cause each peer to get one request each six seconds for the various assets we need
}
}
Expand Down
Loading