[TierTwo] Fix and improve several synchronization problems

src/budget/budgetmanager.h

@@ -12,6 +12,8 @@
 
 class CValidationState;
 
+#define ORPHAN_VOTES_CACHE_LIMIT 10000
+
 //
 // Budget Manager : Contains all proposals for the budget
 //
@@ -31,9 +33,11 @@ class CBudgetManager : public CValidationInterface
     std::map<uint256, CFinalizedBudget> mapFinalizedBudgets;                // guarded by cs_budgets
 
     std::map<uint256, CBudgetVote> mapSeenProposalVotes;                    // guarded by cs_votes
-    std::map<uint256, CBudgetVote> mapOrphanProposalVotes;                  // guarded by cs_votes
+    typedef std::pair<std::vector<CBudgetVote>, int64_t> PropVotesAndLastVoteReceivedTime;
+    std::map<uint256, PropVotesAndLastVoteReceivedTime> mapOrphanProposalVotes;        // guarded by cs_votes
     std::map<uint256, CFinalizedBudgetVote> mapSeenFinalizedBudgetVotes;    // guarded by cs_finalizedvotes
-    std::map<uint256, CFinalizedBudgetVote> mapOrphanFinalizedBudgetVotes;  // guarded by cs_finalizedvotes
+    typedef std::pair<std::vector<CFinalizedBudgetVote>, int64_t> BudVotesAndLastVoteReceivedTime;
+    std::map<uint256, BudVotesAndLastVoteReceivedTime> mapOrphanFinalizedBudgetVotes;  // guarded by cs_finalizedvotes
 
     // Memory Only. Updated in NewBlock (blocks arrive in order)
     std::atomic<int> nBestHeight;
@@ -62,19 +66,16 @@ class CBudgetManager : public CValidationInterface
 
     CBudgetManager() {}
 
-    void ClearSeen()
-    {
-        WITH_LOCK(cs_votes, mapSeenProposalVotes.clear(); );
-        WITH_LOCK(cs_finalizedvotes, mapSeenFinalizedBudgetVotes.clear(); );
-    }
-
     void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
 
     bool HaveProposal(const uint256& propHash) const { LOCK(cs_proposals); return mapProposals.count(propHash); }
     bool HaveSeenProposalVote(const uint256& voteHash) const { LOCK(cs_votes); return mapSeenProposalVotes.count(voteHash); }
     bool HaveFinalizedBudget(const uint256& budgetHash) const { LOCK(cs_budgets); return mapFinalizedBudgets.count(budgetHash); }
     bool HaveSeenFinalizedBudgetVote(const uint256& voteHash) const { LOCK(cs_finalizedvotes); return mapSeenFinalizedBudgetVotes.count(voteHash); }
 
+    // Clears and reloads seen votes in the maps, and clears orphan votes
+    void ReloadMapSeen();
+
     void AddSeenProposalVote(const CBudgetVote& vote);
     void AddSeenFinalizedBudgetVote(const CFinalizedBudgetVote& vote);
 
@@ -98,7 +99,10 @@ class CBudgetManager : public CValidationInterface
 
     void ResetSync() { SetSynced(false); }
     void MarkSynced() { SetSynced(true); }
-    void Sync(CNode* node, const uint256& nProp, bool fPartial = false);
+    // Respond to full budget sync requests and internally triggered partial budget items relay
+    void Sync(CNode* node, bool fPartial);
+    // Respond to single budget item requests (proposals / budget finalization)
+    void SyncSingleItem(CNode* pfrom, const uint256& nProp);
     void SetBestHeight(int height) { nBestHeight.store(height, std::memory_order_release); };
     int GetBestHeight() const { return nBestHeight.load(std::memory_order_acquire); }
 
@@ -140,7 +144,7 @@ class CBudgetManager : public CValidationInterface
     uint256 SubmitFinalBudget();
 
     bool UpdateProposal(const CBudgetVote& vote, CNode* pfrom, std::string& strError);
-    bool UpdateFinalizedBudget(CFinalizedBudgetVote& vote, CNode* pfrom, std::string& strError);
+    bool UpdateFinalizedBudget(const CFinalizedBudgetVote& vote, CNode* pfrom, std::string& strError);
     TrxValidationStatus IsTransactionValid(const CTransaction& txNew, const uint256& nBlockHash, int nBlockHeight) const;
     std::string GetRequiredPaymentsString(int nBlockHeight);
     bool FillBlockPayee(CMutableTransaction& txCoinbase, CMutableTransaction& txCoinstake, const int nHeight, bool fProofOfStake) const;

src/budget/budgetproposal.cpp

@@ -239,12 +239,6 @@ bool CBudgetProposal::AddOrUpdateVote(const CBudgetVote& vote, std::string& strE
         strAction = "Existing vote updated:";
     }
 
-    if (voteTime > GetTime() + (60 * 60)) {
-        strError = strprintf("new vote is too far ahead of current time - %s - nTime %lli - Max Time %lli\n", vote.GetHash().ToString(), voteTime, GetTime() + (60 * 60));
-        LogPrint(BCLog::MNBUDGET, "%s: %s\n", __func__, strError);
-        return false;
-    }
-
     mapVotes[mnId] = vote;
     LogPrint(BCLog::MNBUDGET, "%s: %s %s\n", __func__, strAction.c_str(), vote.GetHash().ToString().c_str());
 

src/budget/finalizedbudget.cpp

@@ -73,13 +73,6 @@ bool CFinalizedBudget::AddOrUpdateVote(const CFinalizedBudgetVote& vote, std::st
         strAction = "Existing vote updated:";
     }
 
-    if (voteTime > GetTime() + (60 * 60)) {
-        strError = strprintf("new vote is too far ahead of current time - %s - nTime %lli - Max Time %lli\n",
-                vote.GetHash().ToString(), voteTime, GetTime() + (60 * 60));
-        LogPrint(BCLog::MNBUDGET, "%s: %s\n", __func__, strError);
-        return false;
-    }
-
     mapVotes[mnId] = vote;
     LogPrint(BCLog::MNBUDGET, "%s: %s %s\n", __func__, strAction.c_str(), vote.GetHash().ToString().c_str());
     return true;

src/init.cpp

@@ -1819,7 +1819,7 @@ bool AppInitMain()
 
     //flag our cached items so we send them to our peers
     g_budgetman.ResetSync();
-    g_budgetman.ClearSeen();
+    g_budgetman.ReloadMapSeen();
 
     RegisterValidationInterface(&g_budgetman);
 

src/masternode-sync.cpp

@@ -220,17 +220,12 @@ std::string CMasternodeSync::GetSyncStatus()
     return "";
 }
 
-void CMasternodeSync::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataStream& vRecv)
+void CMasternodeSync::ProcessSyncStatusMsg(int nItemID, int nCount)
 {
-    if (strCommand == NetMsgType::SYNCSTATUSCOUNT) { //Sync status count
-        int nItemID;
-        int nCount;
-        vRecv >> nItemID >> nCount;
+    if (RequestedMasternodeAssets >= MASTERNODE_SYNC_FINISHED) return;
 
-        if (RequestedMasternodeAssets >= MASTERNODE_SYNC_FINISHED) return;
-
-        //this means we will receive no further communication
-        switch (nItemID) {
+    //this means we will receive no further communication
+    switch (nItemID) {
         case (MASTERNODE_SYNC_LIST):
             if (nItemID != RequestedMasternodeAssets) return;
             sumMasternodeList += nCount;
@@ -251,10 +246,11 @@ void CMasternodeSync::ProcessMessage(CNode* pfrom, std::string& strCommand, CDat
             sumBudgetItemFin += nCount;
             countBudgetItemFin++;
             break;
-        }
-
-        LogPrint(BCLog::MASTERNODE, "CMasternodeSync:ProcessMessage - ssc - got inventory count %d %d\n", nItemID, nCount);
+        default:
+            break;
     }
+
+    LogPrint(BCLog::MASTERNODE, "CMasternodeSync:ProcessMessage - ssc - got inventory count %d %d\n", nItemID, nCount);
 }
 
 void CMasternodeSync::ClearFulfilledRequest()
@@ -337,136 +333,154 @@ bool CMasternodeSync::SyncWithNode(CNode* pnode, bool fLegacyMnObsolete)
 
     //set to synced
     if (RequestedMasternodeAssets == MASTERNODE_SYNC_SPORKS) {
+
+        // Sync sporks from at least 2 peers
+        if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD) {
+            SwitchToNextAsset();
+            return false;
+        }
+
+        // Request sporks sync if we haven't requested it yet.
         if (pnode->HasFulfilledRequest("getspork")) return true;
         pnode->FulfilledRequest("getspork");
 
-        g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::GETSPORKS)); //get current network sporks
-        if (RequestedMasternodeAttempt >= 2) SwitchToNextAsset();
+        g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::GETSPORKS));
         RequestedMasternodeAttempt++;
         return false;
     }
 
-    if (pnode->nVersion >= ActiveProtocol()) {
-        if (RequestedMasternodeAssets == MASTERNODE_SYNC_LIST) {
-            if (fLegacyMnObsolete) {
-                SwitchToNextAsset();
-                return false;
-            }
+    if (pnode->nVersion < ActiveProtocol()) {
+        return true; // move to next peer
+    }
+
+    if (RequestedMasternodeAssets == MASTERNODE_SYNC_LIST) {
+        if (fLegacyMnObsolete) {
+            SwitchToNextAsset();
+            return false;
+        }
+
+        LogPrint(BCLog::MASTERNODE, "CMasternodeSync::Process() - lastMasternodeList %lld (GetTime() - MASTERNODE_SYNC_TIMEOUT) %lld\n", lastMasternodeList, GetTime() - MASTERNODE_SYNC_TIMEOUT);
+        if (lastMasternodeList > 0 && lastMasternodeList < GetTime() - MASTERNODE_SYNC_TIMEOUT * 8 && RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD) {
+            // hasn't received a new item in the last 40 seconds AND has sent at least a minimum of MASTERNODE_SYNC_THRESHOLD GETMNLIST requests,
+            // so we'll move to the next asset.
+            SwitchToNextAsset();
+            return false;
+        }
 
-            LogPrint(BCLog::MASTERNODE, "CMasternodeSync::Process() - lastMasternodeList %lld (GetTime() - MASTERNODE_SYNC_TIMEOUT) %lld\n", lastMasternodeList, GetTime() - MASTERNODE_SYNC_TIMEOUT);
-            if (lastMasternodeList > 0 && lastMasternodeList < GetTime() - MASTERNODE_SYNC_TIMEOUT * 8 && RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD) {
-                // hasn't received a new item in the last 40 seconds AND has sent at least a minimum of MASTERNODE_SYNC_THRESHOLD GETMNLIST requests,
-                // so we'll move to the next asset.
+        // timeout
+        if (lastMasternodeList == 0 &&
+            (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
+            if (sporkManager.IsSporkActive(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT)) {
+                syncTimeout("MASTERNODE_SYNC_LIST");
+            } else {
                 SwitchToNextAsset();
-                return false;
             }
+            return false;
+        }
 
-            // timeout
-            if (lastMasternodeList == 0 &&
-                (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
-                if (sporkManager.IsSporkActive(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT)) {
-                    syncTimeout("MASTERNODE_SYNC_LIST");
-                } else {
-                    SwitchToNextAsset();
-                }
-                return false;
-            }
+        // Don't request mnlist initial sync to more than 8 randomly ordered peers in this round
+        if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 4) return false;
 
-            // Don't request mnlist initial sync to more than 8 randomly ordered peers in this round
-            if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 4) return false;
+        // Request mnb sync if we haven't requested it yet.
+        if (pnode->HasFulfilledRequest("mnsync")) return true;
 
-            // Request mnb sync if we haven't requested it yet.
-            if (pnode->HasFulfilledRequest("mnsync")) return true;
+        // Try to request MN list sync.
+        if (!mnodeman.RequestMnList(pnode)) {
+            return true; // Failed, try next peer.
+        }
 
-            // Try to request MN list sync.
-            if (!mnodeman.RequestMnList(pnode)) {
-                return true; // Failed, try next peer.
-            }
+        // Mark sync requested.
+        pnode->FulfilledRequest("mnsync");
+        // Increase the sync attempt count
+        RequestedMasternodeAttempt++;
 
-            // Mark sync requested.
-            pnode->FulfilledRequest("mnsync");
-            // Increase the sync attempt count
-            RequestedMasternodeAttempt++;
+        return false; // sleep 1 second before do another request round.
+    }
 
-            return false; // sleep 1 second before do another request round.
+    if (RequestedMasternodeAssets == MASTERNODE_SYNC_MNW) {
+        if (fLegacyMnObsolete) {
+            SwitchToNextAsset();
+            return false;
         }
 
-        if (RequestedMasternodeAssets == MASTERNODE_SYNC_MNW) {
-            if (fLegacyMnObsolete) {
-                SwitchToNextAsset();
-                return false;
-            }
+        if (lastMasternodeWinner > 0 && lastMasternodeWinner < GetTime() - MASTERNODE_SYNC_TIMEOUT * 2 && RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD) { //hasn't received a new item in the last five seconds, so we'll move to the
+            SwitchToNextAsset();
+            // in case we received a budget item while we were syncing the mnw, let's reset the last budget item received time.
+            // reason: if we received for example a single proposal +50 seconds ago, then once the budget sync starts (right after this call),
+            // it will look like the sync is finished, and will not wait to receive any budget data and declare the sync over.
+            lastBudgetItem = 0;
+            return false;
+        }
 
-            if (lastMasternodeWinner > 0 && lastMasternodeWinner < GetTime() - MASTERNODE_SYNC_TIMEOUT * 2 && RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD) { //hasn't received a new item in the last five seconds, so we'll move to the
+        // timeout
+        if (lastMasternodeWinner == 0 &&
+            (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 2 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
+            if (sporkManager.IsSporkActive(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT)) {
+                syncTimeout("MASTERNODE_SYNC_MNW");
+            } else {
                 SwitchToNextAsset();
-                return false;
-            }
-
-            // timeout
-            if (lastMasternodeWinner == 0 &&
-                (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
-                if (sporkManager.IsSporkActive(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT)) {
-                    syncTimeout("MASTERNODE_SYNC_MNW");
-                } else {
-                    SwitchToNextAsset();
-                }
-                return false;
+                // Same as above (future: remove all of this duplicated code in v6.0.)
+                // in case we received a budget item while we were syncing the mnw, let's reset the last budget item received time.
+                // reason: if we received for example a single proposal +50 seconds ago, then once the budget sync starts (right after this call),
+                // it will look like the sync is finished, and will not wait to receive any budget data and declare the sync over.
+                lastBudgetItem = 0;
             }
+            return false;
+        }
 
-            // Don't request mnw initial sync to more than 6 randomly ordered peers in this round.
-            if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3) return false;
+        // Don't request mnw initial sync to more than 4 randomly ordered peers in this round.
+        if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 2) return false;
 
-            // Request mnw sync if we haven't requested it yet.
-            if (pnode->HasFulfilledRequest("mnwsync")) return true;
+        // Request mnw sync if we haven't requested it yet.
+        if (pnode->HasFulfilledRequest("mnwsync")) return true;
 
-            // Mark sync requested.
-            pnode->FulfilledRequest("mnwsync");
+        // Mark sync requested.
+        pnode->FulfilledRequest("mnwsync");
 
-            // Sync mn winners
-            int nMnCount = mnodeman.CountEnabled(true /* only_legacy */);
-            g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::GETMNWINNERS, nMnCount));
-            RequestedMasternodeAttempt++;
+        // Sync mn winners
+        int nMnCount = mnodeman.CountEnabled(true /* only_legacy */);
+        g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::GETMNWINNERS, nMnCount));
+        RequestedMasternodeAttempt++;
 
-            return false; // sleep 1 second before do another request round.
-        }
+        return false; // sleep 1 second before do another request round.
+    }
 
-        if (RequestedMasternodeAssets == MASTERNODE_SYNC_BUDGET) {
-            // We'll start rejecting votes if we accidentally get set as synced too soon
-            if (lastBudgetItem > 0 && lastBudgetItem < GetTime() - MASTERNODE_SYNC_TIMEOUT * 10 && RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD) {
-                // Hasn't received a new item in the last fifty seconds and more than MASTERNODE_SYNC_THRESHOLD requests were sent,
-                // so we'll move to the next asset
-                SwitchToNextAsset();
+    if (RequestedMasternodeAssets == MASTERNODE_SYNC_BUDGET) {
+        // We'll start rejecting votes if we accidentally get set as synced too soon
+        if (lastBudgetItem > 0 && lastBudgetItem < GetTime() - MASTERNODE_SYNC_TIMEOUT * 10 && RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD) {
+            // Hasn't received a new item in the last fifty seconds and more than MASTERNODE_SYNC_THRESHOLD requests were sent,
+            // so we'll move to the next asset
+            SwitchToNextAsset();
 
-                // Try to activate our masternode if possible
-                activeMasternode.ManageStatus();
-                return false;
-            }
+            // Try to activate our masternode if possible
+            activeMasternode.ManageStatus();
+            return false;
+        }
 
-            // timeout
-            if (lastBudgetItem == 0 &&
-                (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
-                // maybe there is no budgets at all, so just finish syncing
-                SwitchToNextAsset();
-                activeMasternode.ManageStatus();
-                return false;
-            }
+        // timeout
+        if (lastBudgetItem == 0 &&
+            (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
+            // maybe there is no budgets at all, so just finish syncing
+            SwitchToNextAsset();
+            activeMasternode.ManageStatus();
+            return false;
+        }
 
-            // Don't request budget initial sync to more than 6 randomly ordered peers in this round.
-            if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3) return false;
+        // Don't request budget initial sync to more than 6 randomly ordered peers in this round.
+        if (RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3) return false;
 
-            // Request bud sync if we haven't requested it yet.
-            if (pnode->HasFulfilledRequest("busync")) return true;
+        // Request bud sync if we haven't requested it yet.
+        if (pnode->HasFulfilledRequest("busync")) return true;
 
-            // Mark sync requested.
-            pnode->FulfilledRequest("busync");
+        // Mark sync requested.
+        pnode->FulfilledRequest("busync");
 
-            // Sync proposals, finalizations and votes
-            uint256 n;
-            g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::BUDGETVOTESYNC, n));
-            RequestedMasternodeAttempt++;
+        // Sync proposals, finalizations and votes
+        uint256 n;
+        g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::BUDGETVOTESYNC, n));
+        RequestedMasternodeAttempt++;
 
-            return false; // sleep 1 second before do another request round.
-        }
+        return false; // sleep 1 second before do another request round.
     }
 
     return true;

src/masternode-sync.h

@@ -79,7 +79,7 @@ class CMasternodeSync
     void AddedBudgetItem(const uint256& hash);
     void SwitchToNextAsset();
     std::string GetSyncStatus();
-    void ProcessMessage(CNode* pfrom, std::string& strCommand, CDataStream& vRecv);
+    void ProcessSyncStatusMsg(int nItemID, int itemCount);
     bool IsBudgetFinEmpty();
     bool IsBudgetPropEmpty();