From dfad32272cea4614c5a624c95dca32ce1cf6667d Mon Sep 17 00:00:00 2001 From: Evan Duffield Date: Wed, 29 Jul 2015 08:28:49 -0700 Subject: [PATCH] Refactored locking for masternode winners and budgets --- src/masternode-budget.cpp | 36 ++++++++++++++++++++++--- src/masternode-budget.h | 13 +++++++-- src/masternode-payments.cpp | 53 ++++++++++++++++++++----------------- src/masternode-payments.h | 13 +++++++++ 4 files changed, 86 insertions(+), 29 deletions(-) diff --git a/src/masternode-budget.cpp b/src/masternode-budget.cpp index 10a9c29af..8b41972a5 100644 --- a/src/masternode-budget.cpp +++ b/src/masternode-budget.cpp @@ -85,6 +85,8 @@ bool IsBudgetCollateralValid(uint256 nTxCollateralHash, uint256 nExpectedHash, s void CBudgetManager::CheckOrphanVotes() { + LOCK(cs); + std::string strError = ""; std::map::iterator it1 = mapOrphanMasternodeBudgetVotes.begin(); while(it1 != mapOrphanMasternodeBudgetVotes.end()){ @@ -181,6 +183,7 @@ void CBudgetManager::SubmitFinalBudget() return; } + LOCK(cs); mapSeenFinalizedBudgets.insert(make_pair(finalizedBudgetBroadcast.GetHash(), finalizedBudgetBroadcast)); finalizedBudgetBroadcast.Relay(); budget.AddFinalizedBudget(finalizedBudgetBroadcast); @@ -198,6 +201,8 @@ CBudgetDB::CBudgetDB() bool CBudgetDB::Write(const CBudgetManager& objToSave) { + LOCK(objToSave.cs); + int64_t nStart = GetTimeMillis(); // serialize, checksum data up to that point, then append checksum @@ -230,6 +235,7 @@ bool CBudgetDB::Write(const CBudgetManager& objToSave) CBudgetDB::ReadResult CBudgetDB::Read(CBudgetManager& objToLoad, bool fDryRun) { + LOCK(objToLoad.cs); int64_t nStart = GetTimeMillis(); // open input file, and associate with CAutoFile @@ -349,7 +355,6 @@ void DumpBudgets() bool CBudgetManager::AddFinalizedBudget(CFinalizedBudget& finalizedBudget) { - LOCK(cs); std::string strError = ""; if(!finalizedBudget.IsValid(strError)) return false; @@ -405,6 +410,8 @@ void CBudgetManager::CheckAndRemove() void CBudgetManager::FillBlockPayee(CMutableTransaction& txNew, CAmount nFees) { + LOCK(cs); + CBlockIndex* pindexPrev = chainActive.Tip(); if(!pindexPrev) return; @@ -480,13 +487,18 @@ CBudgetProposal *CBudgetManager::FindProposal(const std::string &strProposalName CBudgetProposal *CBudgetManager::FindProposal(uint256 nHash) { + LOCK(cs); + if(mapProposals.count(nHash)) return &mapProposals[nHash]; return NULL; } -bool CBudgetManager::IsBudgetPaymentBlock(int nBlockHeight){ +bool CBudgetManager::IsBudgetPaymentBlock(int nBlockHeight) +{ + LOCK(cs); + int nHighestCount = -1; std::map::iterator it = mapFinalizedBudgets.begin(); @@ -529,6 +541,8 @@ bool CBudgetManager::HasNextFinalizedBudget() bool CBudgetManager::IsTransactionValid(const CTransaction& txNew, int nBlockHeight) { + LOCK(cs); + int nHighestCount = 0; std::vector ret; @@ -576,6 +590,8 @@ bool CBudgetManager::IsTransactionValid(const CTransaction& txNew, int nBlockHei std::vector CBudgetManager::GetAllProposals() { + LOCK(cs); + std::vector vBudgetProposalRet; std::map::iterator it = mapProposals.begin(); @@ -606,6 +622,8 @@ struct sortProposalsByVotes { //Need to review this function std::vector CBudgetManager::GetBudget() { + LOCK(cs); + // ------- Sort budgets by Yes Count std::vector > vBudgetPorposalsSort; @@ -667,6 +685,8 @@ struct sortFinalizedBudgetsByVotes { std::vector CBudgetManager::GetFinalizedBudgets() { + LOCK(cs); + std::vector vFinalizedBudgetsRet; std::vector > vFinalizedBudgetsSort; @@ -694,6 +714,8 @@ std::vector CBudgetManager::GetFinalizedBudgets() std::string CBudgetManager::GetRequiredPaymentsString(int nBlockHeight) { + LOCK(cs); + std::string ret = "unknown-budget"; std::map::iterator it = mapFinalizedBudgets.begin(); @@ -743,6 +765,8 @@ CAmount CBudgetManager::GetTotalBudget(int nHeight) void CBudgetManager::NewBlock() { + LOCK(cs); + if (masternodeSync.RequestedMasternodeAssets <= MASTERNODE_SYNC_BUDGET) return; if (strBudgetMode == "suggest") { //suggest the budget we see @@ -935,6 +959,8 @@ bool CBudgetManager::PropExists(uint256 nHash) void CBudgetManager::Sync(CNode* pfrom, uint256 nProp) { + LOCK(cs); + /* Sync with a client on the network @@ -1472,6 +1498,8 @@ bool CFinalizedBudget::AddOrUpdateVote(CFinalizedBudgetVote& vote, std::string& //evaluate if we should vote for this. Masternode only void CFinalizedBudget::AutoCheck() { + LOCK(cs); + if(!fMasterNode || fAutoChecked) return; //do this 1 in 20 blocks -- spread out the voting activity on mainnet @@ -1538,7 +1566,9 @@ CAmount CFinalizedBudget::GetTotalPayout() return ret; } -std::string CFinalizedBudget::GetProposals() { +std::string CFinalizedBudget::GetProposals() +{ + LOCK(cs); std::string ret = ""; BOOST_FOREACH(CTxBudgetPayment& budgetPayment, vecBudgetPayments){ diff --git a/src/masternode-budget.h b/src/masternode-budget.h index ba622f8c7..d47b13f0e 100644 --- a/src/masternode-budget.h +++ b/src/masternode-budget.h @@ -17,6 +17,8 @@ using namespace std; +extern CCriticalSection cs_budget; + class CBudgetManager; class CFinalizedBudgetBroadcast; class CFinalizedBudget; @@ -73,13 +75,14 @@ public: class CBudgetManager { private: - // critical section to protect the inner data structures - mutable CCriticalSection cs; //hold txes until they mature enough to use map mapCollateral; public: + // critical section to protect the inner data structures + mutable CCriticalSection cs; + // keep track of the scanning errors I've seen map mapProposals; map mapFinalizedBudgets; @@ -128,6 +131,8 @@ public: void CheckOrphanVotes(); void Clear(){ + LOCK(cs); + LogPrintf("Budget object cleared\n"); mapProposals.clear(); mapFinalizedBudgets.clear(); @@ -221,6 +226,8 @@ public: bool IsTransactionValid(const CTransaction& txNew, int nBlockHeight); bool GetBudgetPaymentByBlock(int64_t nBlockHeight, CTxBudgetPayment& payment) { + LOCK(cs); + int i = nBlockHeight - GetBlockStart(); if(i < 0) return false; if(i > (int)vecBudgetPayments.size() - 1) return false; @@ -229,6 +236,8 @@ public: } bool GetPayeeAndAmount(int64_t nBlockHeight, CScript& payee, CAmount& nAmount) { + LOCK(cs); + int i = nBlockHeight - GetBlockStart(); if(i < 0) return false; if(i > (int)vecBudgetPayments.size() - 1) return false; diff --git a/src/masternode-payments.cpp b/src/masternode-payments.cpp index 7be2f2b48..3452a4a1b 100644 --- a/src/masternode-payments.cpp +++ b/src/masternode-payments.cpp @@ -14,11 +14,13 @@ #include #include -CCriticalSection cs_masternodepayments; - /** Object for who's going to get paid on which blocks */ CMasternodePayments masternodePayments; +CCriticalSection cs_vecPayments; +CCriticalSection cs_mapMasternodeBlocks; +CCriticalSection cs_mapMasternodePayeeVotes; + // // CMasternodePaymentDB // @@ -353,9 +355,6 @@ void CMasternodePayments::ProcessMessageMasternodePayments(CNode* pfrom, std::st LogPrintf("mnget - Sent Masternode winners to %s\n", pfrom->addr.ToString().c_str()); } else if (strCommand == "mnw") { //Masternode Payments Declare Winner - // disabled due to locking issues - LOCK(cs_masternodepayments); - //this is required in litemodef CMasternodePaymentWinner winner; vRecv >> winner; @@ -439,6 +438,8 @@ bool CMasternodePayments::GetBlockPayee(int nBlockHeight, CScript& payee) // -- Only look ahead up to 8 blocks to allow for propagation of the latest 2 winners bool CMasternodePayments::IsScheduled(CMasternode& mn, int nNotBlockHeight) { + LOCK(cs_mapMasternodeBlocks); + CBlockIndex* pindexPrev = chainActive.Tip(); if(pindexPrev == NULL) return false; @@ -467,15 +468,19 @@ bool CMasternodePayments::AddWinningMasternode(CMasternodePaymentWinner& winnerI return false; } - if(mapMasternodePayeeVotes.count(winnerIn.GetHash())){ - return false; - } + { + LOCK2(cs_mapMasternodePayeeVotes, cs_mapMasternodeBlocks); + + if(mapMasternodePayeeVotes.count(winnerIn.GetHash())){ + return false; + } - mapMasternodePayeeVotes[winnerIn.GetHash()] = winnerIn; + mapMasternodePayeeVotes[winnerIn.GetHash()] = winnerIn; - if(!mapMasternodeBlocks.count(winnerIn.nBlockHeight)){ - CMasternodeBlockPayees blockPayees(winnerIn.nBlockHeight); - mapMasternodeBlocks[winnerIn.nBlockHeight] = blockPayees; + if(!mapMasternodeBlocks.count(winnerIn.nBlockHeight)){ + CMasternodeBlockPayees blockPayees(winnerIn.nBlockHeight); + mapMasternodeBlocks[winnerIn.nBlockHeight] = blockPayees; + } } int n = 1; @@ -487,6 +492,8 @@ bool CMasternodePayments::AddWinningMasternode(CMasternodePaymentWinner& winnerI bool CMasternodeBlockPayees::IsTransactionValid(const CTransaction& txNew) { + LOCK(cs_vecPayments); + int nMaxSignatures = 0; std::string strPayeesPossible = ""; @@ -532,6 +539,8 @@ bool CMasternodeBlockPayees::IsTransactionValid(const CTransaction& txNew) std::string CMasternodeBlockPayees::GetRequiredPaymentsString() { + LOCK(cs_vecPayments); + std::string ret = "Unknown"; BOOST_FOREACH(CMasternodePayee& payee, vecPayments) @@ -552,6 +561,8 @@ std::string CMasternodeBlockPayees::GetRequiredPaymentsString() std::string CMasternodePayments::GetRequiredPaymentsString(int nBlockHeight) { + LOCK(cs_mapMasternodeBlocks); + if(mapMasternodeBlocks.count(nBlockHeight)){ return mapMasternodeBlocks[nBlockHeight].GetRequiredPaymentsString(); } @@ -561,6 +572,8 @@ std::string CMasternodePayments::GetRequiredPaymentsString(int nBlockHeight) bool CMasternodePayments::IsTransactionValid(const CTransaction& txNew, int nBlockHeight) { + LOCK(cs_mapMasternodeBlocks); + if(mapMasternodeBlocks.count(nBlockHeight)){ return mapMasternodeBlocks[nBlockHeight].IsTransactionValid(txNew); } @@ -570,7 +583,7 @@ bool CMasternodePayments::IsTransactionValid(const CTransaction& txNew, int nBlo void CMasternodePayments::CleanPaymentList() { - LOCK(cs_masternodepayments); + LOCK(cs_mapMasternodePayeeVotes); if(chainActive.Tip() == NULL) return; @@ -634,8 +647,6 @@ bool CMasternodePaymentWinner::IsValid(std::string& strError) bool CMasternodePayments::ProcessBlock(int nBlockHeight) { - LOCK(cs_masternodepayments); - if(!fMasterNode) return false; //reference node - hybrid mode @@ -663,12 +674,6 @@ bool CMasternodePayments::ProcessBlock(int nBlockHeight) if(budget.IsBudgetPaymentBlock(nBlockHeight)){ //is budget payment block -- handled by the budgeting software } else { - uint256 hash; - - if(!GetBlockHash(hash, nBlockHeight-100)) return false; - unsigned int nHash; - memcpy(&nHash, &hash, 2); - LogPrintf("CMasternodePayments::ProcessBlock() Start nHeight %d - vin %s. \n", nBlockHeight, activeMasternode.vin.ToString().c_str()); // pay to the oldest MN that still had no payment but its input is old enough and it was active long enough @@ -751,7 +756,7 @@ bool CMasternodePaymentWinner::SignatureValid() void CMasternodePayments::Sync(CNode* node, int nCountNeeded) { - LOCK(cs_masternodepayments); + LOCK(cs_mapMasternodePayeeVotes); if(chainActive.Tip() == NULL) return; @@ -786,7 +791,7 @@ std::string CMasternodePayments::ToString() const int CMasternodePayments::GetOldestBlock() { - LOCK(cs_masternodepayments); + LOCK(cs_mapMasternodeBlocks); int nOldestBlock = std::numeric_limits::max(); @@ -805,7 +810,7 @@ int CMasternodePayments::GetOldestBlock() int CMasternodePayments::GetNewestBlock() { - LOCK(cs_masternodepayments); + LOCK(cs_mapMasternodeBlocks); int nNewestBlock = 0; diff --git a/src/masternode-payments.h b/src/masternode-payments.h index f7939c7ca..21d937420 100644 --- a/src/masternode-payments.h +++ b/src/masternode-payments.h @@ -13,6 +13,10 @@ using namespace std; +extern CCriticalSection cs_vecPayments; +extern CCriticalSection cs_mapMasternodeBlocks; +extern CCriticalSection cs_mapMasternodePayeeVotes; + class CMasternodePayments; class CMasternodePaymentWinner; class CMasternodeBlockPayees; @@ -96,6 +100,8 @@ public: } void AddPayee(CScript payeeIn, int nIncrement){ + LOCK(cs_vecPayments); + BOOST_FOREACH(CMasternodePayee& payee, vecPayments){ if(payee.scriptPubKey == payeeIn) { payee.nVotes += nIncrement; @@ -109,6 +115,8 @@ public: bool GetPayee(CScript& payee) { + LOCK(cs_vecPayments); + int nVotes = -1; BOOST_FOREACH(CMasternodePayee& p, vecPayments){ if(p.nVotes > nVotes){ @@ -122,6 +130,8 @@ public: bool HasPayeeWithVotes(CScript payee, int nVotesReq) { + LOCK(cs_vecPayments); + BOOST_FOREACH(CMasternodePayee& p, vecPayments){ if(p.nVotes >= nVotesReq && p.scriptPubKey == payee) return true; } @@ -225,6 +235,7 @@ public: } void Clear() { + LOCK2(cs_mapMasternodeBlocks, cs_mapMasternodePayeeVotes); mapMasternodeBlocks.clear(); mapMasternodePayeeVotes.clear(); } @@ -241,6 +252,8 @@ public: bool IsScheduled(CMasternode& mn, int nNotBlockHeight); bool CanVote(COutPoint outMasternode, int nBlockHeight) { + LOCK(cs_mapMasternodePayeeVotes); + if(mapMasternodesLastVote.count(outMasternode.hash + outMasternode.n)) { if(mapMasternodesLastVote[outMasternode.hash + outMasternode.n] == nBlockHeight) { return false;