adjust privatesend formatting and follow some best practices (#2979)

* adjust privatesend files formatting and follow some best practices

* code review

Signed-off-by: Pasta <Pasta@dash.org>

* use auto for iterators

Signed-off-by: Pasta <Pasta@dash.org>

* review pt2

Signed-off-by: Pasta <Pasta@dash.org>

src/privatesend/privatesend-client.cpp

@@ -99,7 +99,7 @@ void CPrivateSendClientManager::ProcessMessage(CNode* pfrom, const std::string&
             int64_t nLastDsq = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
             int nThreshold = nLastDsq + mnList.GetValidMNsCount() / 5;
             LogPrint(BCLog::PRIVATESEND, "DSQUEUE -- nLastDsq: %d  threshold: %d  nDsqCount: %d\n", nLastDsq, nThreshold, mmetaman.GetDsqCount());
-            //don't allow a few nodes to dominate the queuing process
+            // don't allow a few nodes to dominate the queuing process
             if (nLastDsq != 0 && nThreshold > mmetaman.GetDsqCount()) {
                 LogPrint(BCLog::PRIVATESEND, "DSQUEUE -- Masternode %s is sending too many dsq messages\n", dmn->proTxHash.ToString());
                 return;
@@ -118,10 +118,9 @@ void CPrivateSendClientManager::ProcessMessage(CNode* pfrom, const std::string&
             dsq.Relay(connman);
         }
 
-    } else if (
+    } else if (strCommand == NetMsgType::DSSTATUSUPDATE ||
-        strCommand == NetMsgType::DSSTATUSUPDATE ||
+               strCommand == NetMsgType::DSFINALTX ||
-        strCommand == NetMsgType::DSFINALTX ||
+               strCommand == NetMsgType::DSCOMPLETE) {
-        strCommand == NetMsgType::DSCOMPLETE) {
         LOCK(cs_deqsessions);
         for (auto& session : deqSessions) {
             session.ProcessMessage(pfrom, strCommand, vRecv, connman);
@@ -201,7 +200,7 @@ void CPrivateSendClientSession::ProcessMessage(CNode* pfrom, const std::string&
 
         LogPrint(BCLog::PRIVATESEND, "DSFINALTX -- txNew %s", txNew.ToString());
 
-        //check to see if input is spent already? (and probably not confirmed)
+        // check to see if input is spent already? (and probably not confirmed)
         SignFinalTransaction(txNew, pfrom, connman);
 
     } else if (strCommand == NetMsgType::DSCOMPLETE) {
@@ -294,8 +293,9 @@ std::string CPrivateSendClientSession::GetStatus(bool fWaitForBlock)
     nStatusMessageProgress += 10;
     std::string strSuffix = "";
 
-    if (fWaitForBlock || !masternodeSync.IsBlockchainSynced())
+    if (fWaitForBlock || !masternodeSync.IsBlockchainSynced()) {
         return strAutoDenomResult;
+    }
 
     switch (nState) {
     case POOL_STATE_IDLE:
@@ -432,8 +432,7 @@ bool CPrivateSendClientSession::CheckTimeout()
     int nTimeout = (nState == POOL_STATE_SIGNING) ? PRIVATESEND_SIGNING_TIMEOUT : PRIVATESEND_QUEUE_TIMEOUT;
     bool fTimeout = GetTime() - nTimeLastSuccessfulStep >= nTimeout + nLagTime;
 
-    if (nState == POOL_STATE_IDLE || !fTimeout)
+    if (nState == POOL_STATE_IDLE || !fTimeout) return false;
-        return false;
 
     LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::CheckTimeout -- %s timed out (%ds) -- resetting\n",
         (nState == POOL_STATE_SIGNING) ? "Signing" : "Session", nTimeout);
@@ -481,11 +480,13 @@ bool CPrivateSendClientSession::SendDenominate(const std::vector<std::pair<CTxDS
     }
 
     // lock the funds we're going to use
-    for (const auto& txin : txMyCollateral.vin)
+    for (const auto& txin : txMyCollateral.vin) {
         vecOutPointLocked.push_back(txin.prevout);
+    }
 
-    for (const auto& pair : vecPSInOutPairsIn)
+    for (const auto& pair : vecPSInOutPairsIn) {
         vecOutPointLocked.push_back(pair.first.prevout);
+    }
 
     // we should already be connected to a Masternode
     if (!nSessionID) {
@@ -630,8 +631,9 @@ bool CPrivateSendClientSession::SignFinalTransaction(const CTransaction& finalTr
                     }
                 }
 
-                for (const auto& txout : entry.vecTxOut)
+                for (const auto& txout : entry.vecTxOut) {
                     nValue2 += txout.nValue;
+                }
 
                 int nTargetOuputsCount = entry.vecTxOut.size();
                 if (nFoundOutputsCount < nTargetOuputsCount || nValue1 != nValue2) {
@@ -714,11 +716,9 @@ void CPrivateSendClientManager::AddSkippedDenom(const CAmount& nDenomValue)
 
 bool CPrivateSendClientManager::WaitForAnotherBlock()
 {
-    if (!masternodeSync.IsBlockchainSynced())
+    if (!masternodeSync.IsBlockchainSynced()) return true;
-        return true;
 
-    if (fPrivateSendMultiSession)
+    if (fPrivateSendMultiSession) return false;
-        return false;
 
     return nCachedBlockHeight - nCachedLastSuccessBlock < nMinBlocksToWait;
 }
@@ -921,8 +921,9 @@ bool CPrivateSendClientSession::DoAutomaticDenominating(CConnman& connman, bool
         }
 
         //check if we have the collateral sized inputs
-        if (!pwalletMain->HasCollateralInputs())
+        if (!pwalletMain->HasCollateralInputs()) {
             return !pwalletMain->HasCollateralInputs(false) && MakeCollateralAmounts(connman);
+        }
 
         if (nSessionID) {
             strAutoDenomResult = _("Mixing in progress...");
@@ -962,14 +963,14 @@ bool CPrivateSendClientSession::DoAutomaticDenominating(CConnman& connman, bool
 
     bool fUseQueue = GetRandInt(100) > 33;
     // don't use the queues all of the time for mixing unless we are a liquidity provider
-    if ((privateSendClient.nLiquidityProvider || fUseQueue) && JoinExistingQueue(nBalanceNeedsAnonymized, connman))
+    if ((privateSendClient.nLiquidityProvider || fUseQueue) && JoinExistingQueue(nBalanceNeedsAnonymized, connman)) {
         return true;
+    }
 
     // do not initiate queue if we are a liquidity provider to avoid useless inter-mixing
     if (privateSendClient.nLiquidityProvider) return false;
 
-    if (StartNewQueue(nBalanceNeedsAnonymized, connman))
+    if (StartNewQueue(nBalanceNeedsAnonymized, connman)) return true;
-        return true;
 
     strAutoDenomResult = _("No compatible Masternode found.");
     return false;
@@ -1013,8 +1014,7 @@ bool CPrivateSendClientManager::DoAutomaticDenominating(CConnman& connman, bool
         deqSessions.emplace_back();
     }
     for (auto& session : deqSessions) {
-        if (!CheckAutomaticBackup())
+        if (!CheckAutomaticBackup()) return false;
-            return false;
 
         if (WaitForAnotherBlock()) {
             LogPrintf("CPrivateSendClientManager::DoAutomaticDenominating -- Last successful PrivateSend action was too recent\n");
@@ -1456,8 +1456,9 @@ bool CPrivateSendClientSession::MakeCollateralAmounts(const CompactTallyItem& ta
     LOCK2(cs_main, pwalletMain->cs_wallet);
 
     // denominated input is always a single one, so we can check its amount directly and return early
-    if (!fTryDenominated && tallyItem.vecOutPoints.size() == 1 && CPrivateSend::IsDenominatedAmount(tallyItem.nAmount))
+    if (!fTryDenominated && tallyItem.vecOutPoints.size() == 1 && CPrivateSend::IsDenominatedAmount(tallyItem.nAmount)) {
         return false;
+    }
 
     CWalletTx wtx;
     CAmount nFeeRet = 0;
@@ -1483,8 +1484,9 @@ bool CPrivateSendClientSession::MakeCollateralAmounts(const CompactTallyItem& ta
     coinControl.fAllowWatchOnly = false;
     // send change to the same address so that we were able create more denoms out of it later
     coinControl.destChange = tallyItem.txdest;
-    for (const auto& outpoint : tallyItem.vecOutPoints)
+    for (const auto& outpoint : tallyItem.vecOutPoints) {
         coinControl.Select(outpoint);
+    }
 
     bool fSuccess = pwalletMain->CreateTransaction(vecSend, wtx, reservekeyChange,
         nFeeRet, nChangePosRet, strFail, &coinControl, true, ONLY_NONDENOMINATED);
@@ -1643,8 +1645,9 @@ bool CPrivateSendClientSession::CreateDenominated(CAmount nBalanceToDenominate,
     coinControl.fAllowWatchOnly = false;
     // send change to the same address so that we were able create more denoms out of it later
     coinControl.destChange = tallyItem.txdest;
-    for (const auto& outpoint : tallyItem.vecOutPoints)
+    for (const auto& outpoint : tallyItem.vecOutPoints) {
         coinControl.Select(outpoint);
+    }
 
     CWalletTx wtx;
     CAmount nFeeRet = 0;
@@ -1705,8 +1708,7 @@ void CPrivateSendClientManager::DoMaintenance(CConnman& connman)
     if (fLiteMode) return;       // disable all Dash specific functionality
     if (fMasternodeMode) return; // no client-side mixing on masternodes
 
-    if (!masternodeSync.IsBlockchainSynced() || ShutdownRequested())
+    if (!masternodeSync.IsBlockchainSynced() || ShutdownRequested()) return;
-        return;
 
     static unsigned int nTick = 0;
     static unsigned int nDoAutoNextRun = nTick + PRIVATESEND_AUTO_TIMEOUT_MIN;

src/privatesend/privatesend-server.cpp

@@ -55,7 +55,7 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm
             return;
         }
 
-        if (vecSessionCollaterals.size() == 0) {
+        if (vecSessionCollaterals.empty()) {
             {
                 TRY_LOCK(cs_vecqueue, lockRecv);
                 if (!lockRecv) return;
@@ -185,14 +185,14 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm
             return;
         }
 
-        //do we have the same denominations as the current session?
+        // do we have the same denominations as the current session?
         if (!IsOutputsCompatibleWithSessionDenom(entry.vecTxOut)) {
             LogPrintf("DSVIN -- not compatible with existing transactions!\n");
             PushStatus(pfrom, STATUS_REJECTED, ERR_EXISTING_TX, connman);
             return;
         }
 
-        //check it like a transaction
+        // check it like a transaction
         {
             CAmount nValueIn = 0;
             CAmount nValueOut = 0;
@@ -343,11 +343,12 @@ void CPrivateSendServer::CreateFinalTransaction(CConnman& connman)
 
     // make our new transaction
     for (int i = 0; i < GetEntriesCount(); i++) {
-        for (const auto& txout : vecEntries[i].vecTxOut)
+        for (const auto& txout : vecEntries[i].vecTxOut) {
             txNew.vout.push_back(txout);
-
+        }
-        for (const auto& txdsin : vecEntries[i].vecTxDSIn)
+        for (const auto& txdsin : vecEntries[i].vecTxDSIn) {
             txNew.vin.push_back(txdsin);
+        }
     }
 
     sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69());
@@ -375,7 +376,7 @@ void CPrivateSendServer::CommitFinalTransaction(CConnman& connman)
         TRY_LOCK(cs_main, lockMain);
         CValidationState validationState;
         mempool.PrioritiseTransaction(hashTx, 0.1 * COIN);
-        if (!lockMain || !AcceptToMemoryPool(mempool, validationState, finalTransaction, false, NULL, false, maxTxFee, true)) {
+        if (!lockMain || !AcceptToMemoryPool(mempool, validationState, finalTransaction, false, nullptr, false, maxTxFee, true)) {
             LogPrintf("CPrivateSendServer::CommitFinalTransaction -- AcceptToMemoryPool() error: Transaction not valid\n");
             SetNull();
             // not much we can do in this case, just notify clients
@@ -433,9 +434,12 @@ void CPrivateSendServer::ChargeFees(CConnman& connman)
     if (nState == POOL_STATE_ACCEPTING_ENTRIES) {
         for (const auto& txCollateral : vecSessionCollaterals) {
             bool fFound = false;
-            for (const auto& entry : vecEntries)
+            for (const auto& entry : vecEntries) {
-                if (*entry.txCollateral == *txCollateral)
+                if (*entry.txCollateral == *txCollateral) {
                     fFound = true;
+                    break;
+                }
+            }
 
             // This queue entry didn't send us the promised transaction
             if (!fFound) {
@@ -584,9 +588,9 @@ bool CPrivateSendServer::IsInputScriptSigValid(const CTxIn& txin)
     CScript sigPubKey = CScript();
 
     for (const auto& entry : vecEntries) {
-        for (const auto& txout : entry.vecTxOut)
+        for (const auto& txout : entry.vecTxOut) {
             txNew.vout.push_back(txout);
-
+        }
         for (const auto& txdsin : entry.vecTxDSIn) {
             txNew.vin.push_back(txdsin);
 
@@ -702,9 +706,11 @@ bool CPrivateSendServer::AddScriptSig(const CTxIn& txinNew)
 // Check to make sure everything is signed
 bool CPrivateSendServer::IsSignaturesComplete()
 {
-    for (const auto& entry : vecEntries)
+    for (const auto& entry : vecEntries) {
-        for (const auto& txdsin : entry.vecTxDSIn)
+        for (const auto& txdsin : entry.vecTxDSIn) {
             if (!txdsin.fHasSig) return false;
+        }
+    }
 
     return true;
 }
@@ -827,7 +833,7 @@ void CPrivateSendServer::RelayFinalTransaction(const CTransaction& txFinal, CCon
         __func__, nSessionID, nSessionDenom, CPrivateSend::GetDenominationsToString(nSessionDenom));
 
     // final mixing tx with empty signatures should be relayed to mixing participants only
-    for (const auto entry : vecEntries) {
+    for (const auto& entry : vecEntries) {
         bool fOk = connman.ForNode(entry.addr, [&txFinal, &connman, this](CNode* pnode) {
             CNetMsgMaker msgMaker(pnode->GetSendVersion());
             connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSFINALTX, nSessionID, txFinal));
@@ -852,7 +858,7 @@ void CPrivateSendServer::RelayStatus(PoolStatusUpdate nStatusUpdate, CConnman& c
 {
     unsigned int nDisconnected{};
     // status updates should be relayed to mixing participants only
-    for (const auto entry : vecEntries) {
+    for (const auto& entry : vecEntries) {
         // make sure everyone is still connected
         bool fOk = connman.ForNode(entry.addr, [&nStatusUpdate, &nMessageID, &connman, this](CNode* pnode) {
             PushStatus(pnode, nStatusUpdate, nMessageID, connman);
@@ -923,8 +929,7 @@ void CPrivateSendServer::DoMaintenance(CConnman& connman)
     if (fLiteMode) return;        // disable all Dash specific functionality
     if (!fMasternodeMode) return; // only run on masternodes
 
-    if (!masternodeSync.IsBlockchainSynced() || ShutdownRequested())
+    if (!masternodeSync.IsBlockchainSynced() || ShutdownRequested()) return;
-        return;
 
     privateSendServer.CheckTimeout(connman);
     privateSendServer.CheckForCompleteQueue(connman);

src/privatesend/privatesend-util.cpp

@@ -46,7 +46,7 @@ void CKeyHolderStorage::KeepAll()
         std::swap(storage, tmp);
     }
 
-    if (tmp.size() > 0) {
+    if (!tmp.empty()) {
         for (auto& key : tmp) {
             key->KeepKey();
         }
@@ -63,7 +63,7 @@ void CKeyHolderStorage::ReturnAll()
         std::swap(storage, tmp);
     }
 
-    if (tmp.size() > 0) {
+    if (!tmp.empty()) {
         for (auto& key : tmp) {
             key->ReturnKey();
         }

src/privatesend/privatesend.cpp

@@ -74,8 +74,9 @@ bool CPrivateSendQueue::Relay(CConnman& connman)
 {
     connman.ForEachNode([&connman, this](CNode* pnode) {
         CNetMsgMaker msgMaker(pnode->GetSendVersion());
-        if (pnode->nVersion >= MIN_PRIVATESEND_PEER_PROTO_VERSION && pnode->fSendDSQueue)
+        if (pnode->nVersion >= MIN_PRIVATESEND_PEER_PROTO_VERSION && pnode->fSendDSQueue) {
             connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSQUEUE, (*this)));
+        }
     });
     return true;
 }
@@ -89,7 +90,6 @@ bool CPrivateSendBroadcastTx::Sign()
 {
     if (!fMasternodeMode) return false;
 
-
     uint256 hash = GetSignatureHash();
 
     CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash);
@@ -103,7 +103,6 @@ bool CPrivateSendBroadcastTx::Sign()
 
 bool CPrivateSendBroadcastTx::CheckSignature(const CBLSPublicKey& blsPubKey) const
 {
-
     uint256 hash = GetSignatureHash();
 
     CBLSSignature sig;
@@ -170,13 +169,14 @@ void CPrivateSendBaseManager::CheckQueue()
     if (!lockDS) return; // it's ok to fail here, we run this quite frequently
 
     // check mixing queue objects for timeouts
-    std::vector<CPrivateSendQueue>::iterator it = vecPrivateSendQueue.begin();
+    auto it = vecPrivateSendQueue.begin();
     while (it != vecPrivateSendQueue.end()) {
         if ((*it).IsExpired()) {
             LogPrint(BCLog::PRIVATESEND, "CPrivateSendBaseManager::%s -- Removing expired queue (%s)\n", __func__, (*it).ToString());
             it = vecPrivateSendQueue.erase(it);
-        } else
+        } else {
             ++it;
+        }
     }
 }
 
@@ -350,8 +350,9 @@ int CPrivateSend::GetDenominations(const std::vector<CTxOut>& vecTxOut, bool fSi
     std::vector<std::pair<CAmount, int> > vecDenomUsed;
 
     // make a list of denominations, with zero uses
-    for (const auto& nDenomValue : vecStandardDenominations)
+    for (const auto& nDenomValue : vecStandardDenominations) {
         vecDenomUsed.push_back(std::make_pair(nDenomValue, 0));
+    }
 
     // look for denominations and update uses to 1
     for (const auto& txout : vecTxOut) {
@@ -415,9 +416,9 @@ int CPrivateSend::GetDenominationsByAmounts(const std::vector<CAmount>& vecAmoun
 
 bool CPrivateSend::IsDenominatedAmount(CAmount nInputAmount)
 {
-    for (const auto& nDenomValue : vecStandardDenominations)
+    for (const auto& nDenomValue : vecStandardDenominations) {
-        if (nInputAmount == nDenomValue)
+        if (nInputAmount == nDenomValue) return true;
-            return true;
+    }
     return false;
 }
 
@@ -450,8 +451,6 @@ std::string CPrivateSend::GetMessageByID(PoolMessage nMessageID)
         return _("Incompatible mode.");
     case ERR_NON_STANDARD_PUBKEY:
         return _("Non-standard public key detected.");
-    case ERR_NOT_A_MN:
-        return _("This is not a Masternode."); // not used
     case ERR_QUEUE_FULL:
         return _("Masternode queue is full.");
     case ERR_RECENT:
@@ -491,7 +490,7 @@ CPrivateSendBroadcastTx CPrivateSend::GetDSTX(const uint256& hash)
 void CPrivateSend::CheckDSTXes(int nHeight)
 {
     LOCK(cs_mapdstx);
-    std::map<uint256, CPrivateSendBroadcastTx>::iterator it = mapDSTX.begin();
+    auto it = mapDSTX.begin();
     while (it != mapDSTX.end()) {
         if (it->second.IsExpired(nHeight)) {
             mapDSTX.erase(it++);

src/privatesend/privatesend.h

@@ -75,7 +75,7 @@ enum PoolStatusUpdate {
     STATUS_ACCEPTED
 };
 
-/** Holds an mixing input
+/** Holds a mixing input
  */
 class CTxDSIn : public CTxIn
 {
@@ -128,7 +128,7 @@ public:
     }
 };
 
-// A clients transaction in the mixing pool
+// A client's transaction in the mixing pool
 class CPrivateSendEntry
 {
 public:
@@ -147,10 +147,10 @@ public:
     }
 
     CPrivateSendEntry(const std::vector<CTxDSIn>& vecTxDSIn, const std::vector<CTxOut>& vecTxOut, const CTransaction& txCollateral) :
-        vecTxDSIn(vecTxDSIn),
+            vecTxDSIn(vecTxDSIn),
-        vecTxOut(vecTxOut),
+            vecTxOut(vecTxOut),
-        txCollateral(MakeTransactionRef(txCollateral)),
+            txCollateral(MakeTransactionRef(txCollateral)),
-        addr(CService())
+            addr(CService())
     {
     }
 
@@ -332,7 +332,7 @@ protected:
     void SetNull();
 
 public:
-    int nSessionDenom; //Users must submit a denom matching this
+    int nSessionDenom; // Users must submit a denom matching this
 
     CPrivateSendBaseSession() :
         vecEntries(),