Refactored locking for masternode winners and budgets

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<uint256, CBudgetVote>::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<uint256, CFinalizedBudget>::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<CFinalizedBudget*> ret;
 
@@ -576,6 +590,8 @@ bool CBudgetManager::IsTransactionValid(const CTransaction& txNew, int nBlockHei
 
 std::vector<CBudgetProposal*> CBudgetManager::GetAllProposals()
 {
+    LOCK(cs);
+
     std::vector<CBudgetProposal*> vBudgetProposalRet;
 
     std::map<uint256, CBudgetProposal>::iterator it = mapProposals.begin();
@@ -606,6 +622,8 @@ struct sortProposalsByVotes {
 //Need to review this function
 std::vector<CBudgetProposal*> CBudgetManager::GetBudget()
 {
+    LOCK(cs);
+
     // ------- Sort budgets by Yes Count
 
     std::vector<std::pair<CBudgetProposal*, int> > vBudgetPorposalsSort;
@@ -667,6 +685,8 @@ struct sortFinalizedBudgetsByVotes {
 
 std::vector<CFinalizedBudget*> CBudgetManager::GetFinalizedBudgets()
 {
+    LOCK(cs);
+
     std::vector<CFinalizedBudget*> vFinalizedBudgetsRet;
     std::vector<std::pair<CFinalizedBudget*, int> > vFinalizedBudgetsSort;
 
@@ -694,6 +714,8 @@ std::vector<CFinalizedBudget*> CBudgetManager::GetFinalizedBudgets()
 
 std::string CBudgetManager::GetRequiredPaymentsString(int nBlockHeight)
 {
+    LOCK(cs);
+
     std::string ret = "unknown-budget";
 
     std::map<uint256, CFinalizedBudget>::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){

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<uint256, CTransaction> mapCollateral;
 
 public:
+    // critical section to protect the inner data structures
+    mutable CCriticalSection cs;
+    
     // keep track of the scanning errors I've seen
     map<uint256, CBudgetProposal> mapProposals;
     map<uint256, CFinalizedBudget> 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;

src/masternode-payments.cpp

@@ -14,11 +14,13 @@
 #include <boost/lexical_cast.hpp>
 #include <boost/filesystem.hpp>
 
-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);
-    }
     
-    mapMasternodePayeeVotes[winnerIn.GetHash()] = winnerIn;
+        if(mapMasternodePayeeVotes.count(winnerIn.GetHash())){
+           return false;
+        }
 
-    if(!mapMasternodeBlocks.count(winnerIn.nBlockHeight)){
+        mapMasternodePayeeVotes[winnerIn.GetHash()] = winnerIn;
-       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<int>::max();
 
@@ -805,7 +810,7 @@ int CMasternodePayments::GetOldestBlock()
 
 int CMasternodePayments::GetNewestBlock()
 {
-    LOCK(cs_masternodepayments);
+    LOCK(cs_mapMasternodeBlocks);
 
     int nNewestBlock = 0;
 

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;