refactor: fix numerous compilation warnings (#4682)

* style: use clang-tidy style named parameters

* refactor: make IsTimeOutOfBounds testable by having current time be a parameter

* style: use x-> not (*x).

* refactor: make SelectCoinsGroupedByAddresses return a vector, remove out param

previous semantics was return false if the vecTally vector was empty. Now we just let the caller check if it is empty or not

* refactor: fix some sign-compare warnings

* refactor: consistently pre-declare stuff as struct / class inline with underlying type

* refactor: don't return const bool

* refactor: use ref to string

* refactor: use = default for CompactTallyItem

* refactor: adjust "initialization" ordering

* refactor: adjust how we handle negatives in GetProjectedMNPayees, use std::min

* refactor: don't bind a reference to a temporary value

* refactor: use a ref

* refactor: ensure attempt in SelectMemberForRecovery is non-negative.

* refactor: remove unused this capture

* refactor: fix numerous sign-compare warnings

* refactor: more consistently use size_t, use empty()

src/bls/bls_worker.cpp

@@ -87,7 +87,7 @@ bool CBLSWorker::GenerateContributions(int quorumThreshold, const BLSIdVector& i
     std::vector<std::future<bool>> futures;
     futures.reserve((quorumThreshold / batchSize + ids.size() / batchSize) + 2);
 
-    for (size_t i = 0; i < quorumThreshold; i += batchSize) {
+    for (size_t i = 0; i < size_t(quorumThreshold); i += batchSize) {
         size_t start = i;
         size_t count = std::min(batchSize, quorumThreshold - start);
         auto f = [&, start, count](int threadId) {
@@ -152,10 +152,10 @@ struct Aggregator : public std::enable_shared_from_this<Aggregator<T>> {
                bool _parallel,
                ctpl::thread_pool& _workerPool,
                DoneCallback _doneCallback) :
+            inputVec(std::make_shared<std::vector<const T*>>(count)),
             parallel(_parallel),
             workerPool(_workerPool),
-            doneCallback(std::move(_doneCallback)),
+            doneCallback(std::move(_doneCallback))
-            inputVec(std::make_shared<std::vector<const T*>>(count))
     {
         for (size_t i = 0; i < count; i++) {
             (*inputVec)[i] = pointer(_inputVec[start + i]);

src/coinjoin/client.cpp

@@ -1055,7 +1055,7 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized,
         }
 
         // skip next mn payments winners
-        if (dmn->pdmnState->nLastPaidHeight + mnList.GetValidMNsCount() < mnList.GetHeight() + winners_to_skip) {
+        if (dmn->pdmnState->nLastPaidHeight + int(mnList.GetValidMNsCount()) < mnList.GetHeight() + winners_to_skip) {
             LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::JoinExistingQueue -- skipping winner, masternode=%s\n", dmn->proTxHash.ToString());
             continue;
         }
@@ -1327,7 +1327,7 @@ bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, s
 
     // NOTE: No need to randomize order of inputs because they were
     // initially shuffled in CWallet::SelectTxDSInsByDenomination already.
-    int nSteps{0};
+    size_t nSteps{0};
     vecPSInOutPairsRet.clear();
 
     // Try to add up to COINJOIN_ENTRY_MAX_SIZE of every needed denomination
@@ -1388,8 +1388,8 @@ bool CCoinJoinClientSession::MakeCollateralAmounts()
     // We still want to consume a lot of inputs to avoid creating only smaller denoms though.
     // Knowing that each CTxIn is at least 148 B big, 400 inputs should take 400 x ~148 B = ~60 kB.
     // This still leaves more than enough room for another data of typical MakeCollateralAmounts tx.
-    std::vector<CompactTallyItem> vecTally;
+    std::vector<CompactTallyItem> vecTally = mixingWallet.SelectCoinsGroupedByAddresses(false, false, true, 400);
-    if (!mixingWallet.SelectCoinsGroupedByAddresses(vecTally, false, false, true, 400)) {
+    if (vecTally.empty()) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::MakeCollateralAmounts -- SelectCoinsGroupedByAddresses can't find any inputs!\n");
         return false;
     }
@@ -1571,8 +1571,8 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate)
     // We still want to consume a lot of inputs to avoid creating only smaller denoms though.
     // Knowing that each CTxIn is at least 148 B big, 400 inputs should take 400 x ~148 B = ~60 kB.
     // This still leaves more than enough room for another data of typical CreateDenominated tx.
-    std::vector<CompactTallyItem> vecTally;
+    std::vector<CompactTallyItem> vecTally = mixingWallet.SelectCoinsGroupedByAddresses(true, true, true, 400);
-    if (!mixingWallet.SelectCoinsGroupedByAddresses(vecTally, true, true, true, 400)) {
+    if (vecTally.empty()) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::CreateDenominated -- SelectCoinsGroupedByAddresses can't find any inputs!\n");
         return false;
     }

src/coinjoin/coinjoin.cpp

@@ -80,9 +80,10 @@ bool CCoinJoinQueue::Relay(CConnman& connman)
     return true;
 }
 
-bool CCoinJoinQueue::IsTimeOutOfBounds() const
+bool CCoinJoinQueue::IsTimeOutOfBounds(int64_t current_time) const
 {
-    return GetAdjustedTime() - nTime > COINJOIN_QUEUE_TIMEOUT || nTime - GetAdjustedTime() > COINJOIN_QUEUE_TIMEOUT;
+    return current_time - nTime > COINJOIN_QUEUE_TIMEOUT ||
+           nTime - current_time > COINJOIN_QUEUE_TIMEOUT;
 }
 
 uint256 CCoinJoinBroadcastTx::GetSignatureHash() const
@@ -129,7 +130,7 @@ bool CCoinJoinBroadcastTx::IsValidStructure() const
     if (tx->vin.size() != tx->vout.size()) {
         return false;
     }
-    if (tx->vin.size() < CCoinJoin::GetMinPoolParticipants()) {
+    if (tx->vin.size() < size_t(CCoinJoin::GetMinPoolParticipants())) {
         return false;
     }
     if (tx->vin.size() > CCoinJoin::GetMaxPoolParticipants() * COINJOIN_ENTRY_MAX_SIZE) {
@@ -167,8 +168,8 @@ void CCoinJoinBaseManager::CheckQueue()
     // check mixing queue objects for timeouts
     auto it = vecCoinJoinQueue.begin();
     while (it != vecCoinJoinQueue.end()) {
-        if ((*it).IsTimeOutOfBounds()) {
+        if (it->IsTimeOutOfBounds()) {
-            LogPrint(BCLog::COINJOIN, "CCoinJoinBaseManager::%s -- Removing a queue (%s)\n", __func__, (*it).ToString());
+            LogPrint(BCLog::COINJOIN, "CCoinJoinBaseManager::%s -- Removing a queue (%s)\n", __func__, it->ToString());
             it = vecCoinJoinQueue.erase(it);
         } else {
             ++it;
@@ -348,7 +349,7 @@ bool CCoinJoin::IsCollateralValid(const CTransaction& txCollateral)
     {
         LOCK(cs_main);
         CValidationState validationState;
-        if (!AcceptToMemoryPool(mempool, validationState, MakeTransactionRef(txCollateral), nullptr /* pfMissingInputs */, false /* bypass_limits */, DEFAULT_MAX_RAW_TX_FEE /* nAbsurdFee */, true /* fDryRun */)) {
+        if (!AcceptToMemoryPool(mempool, validationState, MakeTransactionRef(txCollateral), /*pfMissingInputs=*/nullptr, /*bypass_limits=*/false, /*nAbsurdFee=*/DEFAULT_MAX_RAW_TX_FEE, /*test_accept=*/true)) {
             LogPrint(BCLog::COINJOIN, "CCoinJoin::IsCollateralValid -- didn't pass AcceptToMemoryPool()\n");
             return false;
         }

src/coinjoin/coinjoin.h

@@ -230,7 +230,7 @@ public:
     bool Relay(CConnman& connman);
 
     /// Check if a queue is too old or too far into the future
-    bool IsTimeOutOfBounds() const;
+    bool IsTimeOutOfBounds(int64_t current_time = GetAdjustedTime()) const;
 
     std::string ToString() const
     {

src/coinjoin/server.cpp

@@ -276,7 +276,7 @@ void CCoinJoinServer::CheckPool(CConnman& connman)
     LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %lu\n", GetEntriesCount());
 
     // If we have an entry for each collateral, then create final tx
-    if (nState == POOL_STATE_ACCEPTING_ENTRIES && GetEntriesCount() == vecSessionCollaterals.size()) {
+    if (nState == POOL_STATE_ACCEPTING_ENTRIES && size_t(GetEntriesCount()) == vecSessionCollaterals.size()) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n");
         CreateFinalTransaction(connman);
         return;
@@ -431,10 +431,10 @@ void CCoinJoinServer::ChargeFees(CConnman& connman) const
     if (vecOffendersCollaterals.empty()) return;
 
     //mostly offending? Charge sometimes
-    if ((int)vecOffendersCollaterals.size() >= vecSessionCollaterals.size() - 1 && GetRandInt(100) > 33) return;
+    if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size() - 1 && GetRandInt(100) > 33) return;
 
     //everyone is an offender? That's not right
-    if ((int)vecOffendersCollaterals.size() >= vecSessionCollaterals.size()) return;
+    if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size()) return;
 
     //charge one of the offenders randomly
     Shuffle(vecOffendersCollaterals.begin(), vecOffendersCollaterals.end(), FastRandomContext());
@@ -582,7 +582,7 @@ bool CCoinJoinServer::AddEntry(CConnman& connman, const CCoinJoinEntry& entry, P
 {
     if (!fMasternodeMode) return false;
 
-    if (GetEntriesCount() >= vecSessionCollaterals.size()) {
+    if (size_t(GetEntriesCount()) >= vecSessionCollaterals.size()) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: entries is full!\n", __func__);
         nMessageIDRet = ERR_ENTRIES_FULL;
         return false;

src/evo/cbtx.cpp

@@ -215,7 +215,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
             auto qcHash = ::SerializeHash(qc.commitment);
             const auto& llmq_params = llmq::GetLLMQParams(qc.commitment.llmqType);
             auto& v = qcHashes[llmq_params.type];
-            if (v.size() == llmq_params.signingActiveQuorumCount) {
+            if (v.size() == size_t(llmq_params.signingActiveQuorumCount)) {
                 // we pop the last entry, which is actually the oldest quorum as GetMinedAndActiveCommitmentsUntilBlock
                 // returned quorums in reversed order. This pop and later push can only work ONCE, but we rely on the
                 // fact that a block can only contain a single commitment for one LLMQ type
@@ -223,7 +223,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
             }
             v.emplace_back(qcHash);
             hashCount++;
-            if (v.size() > llmq_params.signingActiveQuorumCount) {
+            if (v.size() > uint64_t(llmq_params.signingActiveQuorumCount)) {
                 return state.DoS(100, false, REJECT_INVALID, "excess-quorums-calc-cbtx-quorummerkleroot");
             }
         }

src/evo/deterministicmns.cpp

@@ -230,9 +230,10 @@ CDeterministicMNCPtr CDeterministicMNList::GetMNPayee() const
 
 std::vector<CDeterministicMNCPtr> CDeterministicMNList::GetProjectedMNPayees(int nCount) const
 {
-    if (nCount > GetValidMNsCount()) {
+    if (nCount < 0 ) {
-        nCount = GetValidMNsCount();
+        return {};
     }
+    nCount = std::min(nCount, int(GetValidMNsCount()));
 
     std::vector<CDeterministicMNCPtr> result;
     result.reserve(nCount);

src/interfaces/chain.h

@@ -29,7 +29,7 @@ enum class MemPoolRemovalReason;
 
 namespace llmq {
 class CChainLockSig;
-class CInstantSendLock;
+struct CInstantSendLock;
 } // namespace llmq
 
 typedef std::shared_ptr<const CTransaction> CTransactionRef;

src/interfaces/wallet.cpp

@@ -556,12 +556,12 @@ public:
     std::unique_ptr<Handler> handleInstantLockReceived(InstantLockReceivedFn fn) override
     {
         return MakeHandler(m_wallet->NotifyISLockReceived.connect(
-            [fn, this]() { fn(); }));
+            [fn]() { fn(); }));
     }
     std::unique_ptr<Handler> handleChainLockReceived(ChainLockReceivedFn fn) override
     {
         return MakeHandler(m_wallet->NotifyChainLockReceived.connect(
-            [fn, this](int chainLockHeight) { fn(chainLockHeight); }));
+            [fn](int chainLockHeight) { fn(chainLockHeight); }));
     }
     std::unique_ptr<Handler> handleWatchOnlyChanged(WatchOnlyChangedFn fn) override
     {

src/llmq/blockprocessor.cpp

@@ -459,7 +459,7 @@ std::vector<const CBlockIndex*> CQuorumBlockProcessor::GetMinedCommitmentsUntilB
         }
 
         uint32_t nMinedHeight = std::numeric_limits<uint32_t>::max() - be32toh(std::get<2>(curKey));
-        if (nMinedHeight > pindex->nHeight) {
+        if (nMinedHeight > uint32_t(pindex->nHeight)) {
             break;
         }
 

src/llmq/commitment.cpp

@@ -69,7 +69,7 @@ bool CFinalCommitment::Verify(const CBlockIndex* pQuorumBaseBlockIndex, bool che
     }
 
     auto members = CLLMQUtils::GetAllQuorumMembers(llmq_params, pQuorumBaseBlockIndex);
-    for (size_t i = members.size(); i < llmq_params.size; i++) {
+    for (size_t i = members.size(); i < size_t(llmq_params.size); i++) {
         if (validMembers[i]) {
             LogPrintfFinalCommitment("invalid validMembers bitset. bit %d should not be set\n", i);
             return false;
@@ -122,11 +122,11 @@ bool CFinalCommitment::VerifyNull() const
 
 bool CFinalCommitment::VerifySizes(const Consensus::LLMQParams& params) const
 {
-    if (signers.size() != params.size) {
+    if (signers.size() != size_t(params.size)) {
         LogPrintfFinalCommitment("invalid signers.size=%d\n", signers.size());
         return false;
     }
-    if (validMembers.size() != params.size) {
+    if (validMembers.size() != size_t(params.size)) {
         LogPrintfFinalCommitment("invalid signers.size=%d\n", signers.size());
         return false;
     }
@@ -144,7 +144,7 @@ bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev,
         return state.DoS(100, false, REJECT_INVALID, "bad-qc-version");
     }
 
-    if (qcTx.nHeight != pindexPrev->nHeight + 1) {
+    if (qcTx.nHeight != uint32_t(pindexPrev->nHeight + 1)) {
         return state.DoS(100, false, REJECT_INVALID, "bad-qc-height");
     }
 

src/llmq/dkgsession.cpp

@@ -98,7 +98,7 @@ bool CDKGSession::Init(const CBlockIndex* _pQuorumBaseBlockIndex, const std::vec
 
     CDKGLogger logger(*this, __func__);
 
-    if (mns.size() < params.minSize) {
+    if (mns.size() < size_t(params.minSize)) {
         logger.Batch("not enough members (%d < %d), aborting init", mns.size(), params.minSize);
         return false;
     }
@@ -213,7 +213,7 @@ bool CDKGSession::PreVerifyMessage(const CDKGContribution& qc, bool& retBan) con
         retBan = true;
         return false;
     }
-    if (qc.vvec->size() != params.threshold) {
+    if (qc.vvec->size() != size_t(params.threshold)) {
         logger.Batch("invalid verification vector length");
         retBan = true;
         return false;
@@ -626,7 +626,7 @@ void CDKGSession::VerifyAndJustify(CDKGPendingMessages& pendingMessages)
         if (m->bad) {
             continue;
         }
-        if (m->badMemberVotes.size() >= params.dkgBadVotesThreshold) {
+        if (m->badMemberVotes.size() >= size_t(params.dkgBadVotesThreshold)) {
             logger.Batch("%s marked as bad as %d other members voted for this", m->dmn->proTxHash.ToString(), m->badMemberVotes.size());
             MarkBadMember(m->idx);
             continue;
@@ -1053,7 +1053,7 @@ bool CDKGSession::PreVerifyMessage(const CDKGPrematureCommitment& qc, bool& retB
         return false;
     }
 
-    for (size_t i = members.size(); i < params.size; i++) {
+    for (size_t i = members.size(); i < size_t(params.size); i++) {
         if (qc.validMembers[i]) {
             retBan = true;
             logger.Batch("invalid validMembers bitset. bit %d should not be set", i);
@@ -1187,7 +1187,7 @@ std::vector<CFinalCommitment> CDKGSession::FinalizeCommitments()
     std::vector<CFinalCommitment> finalCommitments;
     for (const auto& p : commitmentsMap) {
         auto& cvec = p.second;
-        if (cvec.size() < params.minSize) {
+        if (cvec.size() < size_t(params.minSize)) {
             // commitment was signed by a minority
             continue;
         }

src/llmq/instantsend.cpp

@@ -190,7 +190,7 @@ std::unordered_map<uint256, CInstantSendLockPtr, StaticSaltedHasher> CInstantSen
             break;
         }
         uint32_t nHeight = std::numeric_limits<uint32_t>::max() - be32toh(std::get<1>(curKey));
-        if (nHeight > nUntilHeight) {
+        if (nHeight > uint32_t(nUntilHeight)) {
             break;
         }
 
@@ -234,7 +234,7 @@ void CInstantSendDb::RemoveArchivedInstantSendLocks(int nUntilHeight)
             break;
         }
         uint32_t nHeight = std::numeric_limits<uint32_t>::max() - be32toh(std::get<1>(curKey));
-        if (nHeight > nUntilHeight) {
+        if (nHeight > uint32_t(nUntilHeight)) {
             break;
         }
 

src/llmq/quorums.cpp

@@ -674,7 +674,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand,
         // Check if request has ENCRYPTED_CONTRIBUTIONS data
         if (request.GetDataMask() & CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS) {
 
-            if (WITH_LOCK(pQuorum->cs, return pQuorum->quorumVvec->size() != pQuorum->params.threshold)) {
+            if (WITH_LOCK(pQuorum->cs, return pQuorum->quorumVvec->size() != size_t(pQuorum->params.threshold))) {
                 errorHandler("No valid quorum verification vector available", 0); // Don't bump score because we asked for it
                 return;
             }

src/llmq/signing_shares.cpp

@@ -336,7 +336,7 @@ bool CSigSharesManager::ProcessMessageSigSesAnn(const CNode* pfrom, const CSigSe
 
 bool CSigSharesManager::VerifySigSharesInv(Consensus::LLMQType llmqType, const CSigSharesInv& inv)
 {
-    return inv.inv.size() == GetLLMQParams(llmqType).size;
+    return inv.inv.size() == size_t(GetLLMQParams(llmqType).size);
 }
 
 bool CSigSharesManager::ProcessMessageSigSharesInv(const CNode* pfrom, const CSigSharesInv& inv)
@@ -737,7 +737,7 @@ void CSigSharesManager::ProcessSigShare(const CSigShare& sigShare, const CConnma
         }
 
         size_t sigShareCount = sigShares.CountForSignHash(sigShare.GetSignHash());
-        if (sigShareCount >= quorum->params.threshold) {
+        if (sigShareCount >= size_t(quorum->params.threshold)) {
             canTryRecovery = true;
         }
     }
@@ -766,14 +766,14 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256&
 
         sigSharesForRecovery.reserve((size_t) quorum->params.threshold);
         idsForRecovery.reserve((size_t) quorum->params.threshold);
-        for (auto it = sigSharesForSignHash->begin(); it != sigSharesForSignHash->end() && sigSharesForRecovery.size() < quorum->params.threshold; ++it) {
+        for (auto it = sigSharesForSignHash->begin(); it != sigSharesForSignHash->end() && sigSharesForRecovery.size() < size_t(quorum->params.threshold); ++it) {
             auto& sigShare = it->second;
             sigSharesForRecovery.emplace_back(sigShare.sigShare.Get());
             idsForRecovery.emplace_back(quorum->members[sigShare.quorumMember]->proTxHash);
         }
 
         // check if we can recover the final signature
-        if (sigSharesForRecovery.size() < quorum->params.threshold) {
+        if (sigSharesForRecovery.size() < size_t(quorum->params.threshold)) {
             return;
         }
     }
@@ -809,9 +809,9 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256&
     quorumSigningManager->ProcessRecoveredSig(rs);
 }
 
-CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256 &id, int attempt)
+CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256 &id, size_t attempt)
 {
-    assert(attempt < quorum->members.size());
+    assert(size_t(attempt) < quorum->members.size());
 
     std::vector<std::pair<uint256, CDeterministicMNCPtr>> v;
     v.reserve(quorum->members.size());

src/llmq/signing_shares.h

@@ -398,7 +398,7 @@ public:
 
     void HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) override;
 
-    static CDeterministicMNCPtr SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256& id, int attempt);
+    static CDeterministicMNCPtr SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256& id, size_t attempt);
 
 private:
     // all of these return false when the currently processed message should be aborted (as each message actually contains multiple messages)

src/masternode/payments.h

@@ -13,7 +13,7 @@
 class CMasternodePayments;
 class CBlock;
 class CTransaction;
-class CMutableTransaction;
+struct CMutableTransaction;
 class CTxOut;
 
 /// TODO: all 4 functions do not belong here really, they should be refactored/moved somewhere (main.cpp ?)

src/masternode/utils.cpp

@@ -31,7 +31,7 @@ void CMasternodeUtils::ProcessMasternodeConnections(CConnman& connman)
             nonMasternodeCount++;
         }
     });
-    if (nonMasternodeCount < connman.GetMaxOutboundNodeCount()) {
+    if (nonMasternodeCount < int(connman.GetMaxOutboundNodeCount())) {
         return;
     }
 

src/net_processing.cpp

@@ -3919,7 +3919,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
 
     bool found = false;
     const std::vector<std::string> &allMessages = getAllNetMessageTypes();
-    for (const std::string msg : allMessages) {
+    for (const std::string& msg : allMessages) {
         if(msg == strCommand) {
             found = true;
             break;

src/prevector.h

@@ -555,7 +555,7 @@ public:
         // This avoids internal use of std::copy and operator++ on the iterators and instead allows efficient memcpy/memmove
         if (std::is_trivially_constructible<T>::value) {
             auto s = e - b;
-            if (v.size() != s) {
+            if (v.size() != size_t(s)) {
                 v.resize(s);
             }
             if (!v.empty()) {

src/qt/governancelist.cpp

@@ -235,7 +235,7 @@ void ProposalModel::reconcile(const std::vector<const Proposal*>& proposals)
     std::vector<bool> keep_index(m_data.count(), false);
     for (const auto proposal : proposals) {
         bool found = false;
-        for (unsigned int i = 0; i < m_data.count(); ++i) {
+        for (int i = 0; i < m_data.count(); ++i) {
             if (m_data.at(i)->hash() == proposal->hash()) {
                 found = true;
                 keep_index.at(i) = true;

src/qt/guiutil.cpp

@@ -979,7 +979,7 @@ const QString getDefaultTheme()
     return defaultTheme;
 }
 
-const bool isValidTheme(const QString& strTheme)
+bool isValidTheme(const QString& strTheme)
 {
     return strTheme == defaultTheme || strTheme == darkThemePrefix || strTheme == traditionalTheme;
 }
@@ -1626,14 +1626,14 @@ std::vector<QFont::Weight> getSupportedWeights()
 QFont::Weight supportedWeightFromIndex(int nIndex)
 {
     auto vecWeights = getSupportedWeights();
-    assert(vecWeights.size() > nIndex);
+    assert(vecWeights.size() > uint64_t(nIndex));
     return vecWeights[nIndex];
 }
 
 int supportedWeightToIndex(QFont::Weight weight)
 {
     auto vecWeights = getSupportedWeights();
-    for (int index = 0; index < vecWeights.size(); ++index) {
+    for (uint64_t index = 0; index < vecWeights.size(); ++index) {
         if (weight == vecWeights[index]) {
             return index;
         }

src/qt/guiutil.h

@@ -291,7 +291,7 @@ namespace GUIUtil
     const QString getDefaultTheme();
 
     /** Check if the given theme name is valid or not */
-    const bool isValidTheme(const QString& strTheme);
+    bool isValidTheme(const QString& strTheme);
 
     /** Sets the stylesheet of the whole app and updates it if the
     related css files has been changed and -debug-ui mode is active. */

src/rpc/masternode.cpp

@@ -439,7 +439,7 @@ static UniValue masternode_payments(const JSONRPCRequest& request)
     // A temporary vector which is used to sort results properly (there is no "reverse" in/for UniValue)
     std::vector<UniValue> vecPayments;
 
-    while (vecPayments.size() < std::abs(nCount) && pindex != nullptr) {
+    while (vecPayments.size() < uint64_t(std::abs(nCount)) && pindex != nullptr) {
 
         CBlock block;
         if (!ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
@@ -454,7 +454,7 @@ static UniValue masternode_payments(const JSONRPCRequest& request)
                 continue;
             }
             CAmount nValueIn{0};
-            for (const auto txin : tx->vin) {
+            for (const auto& txin : tx->vin) {
                 CTransactionRef txPrev;
                 uint256 blockHashTmp;
                 GetTransaction(txin.prevout.hash, txPrev, Params().GetConsensus(), blockHashTmp);

src/rpc/rpcquorums.cpp

@@ -543,7 +543,7 @@ static UniValue quorum_selectquorum(const JSONRPCRequest& request)
     ret.pushKV("quorumHash", quorum->qc->quorumHash.ToString());
 
     UniValue recoveryMembers(UniValue::VARR);
-    for (int i = 0; i < quorum->params.recoveryMembers; i++) {
+    for (size_t i = 0; i < size_t(quorum->params.recoveryMembers); i++) {
         auto dmn = llmq::quorumSigSharesManager->SelectMemberForRecovery(quorum, id, i);
         recoveryMembers.push_back(dmn->proTxHash.ToString());
     }

src/serialize.h

@@ -513,7 +513,7 @@ void ReadFixedVarIntsBitSet(Stream& s, std::vector<bool>& vec, size_t size)
             break;
         }
         int32_t idx = last + offset;
-        if (idx >= size) {
+        if (idx >= int32_t(size)) {
             throw std::ios_base::failure("out of bounds index");
         }
         if (last != -1 && idx <= last) {

src/stacktraces.cpp

@@ -109,7 +109,7 @@ static std::string GetExeFileName()
         if (len < 0) {
             return "";
         }
-        if (len < buf.size()) {
+        if (len < int64_t(buf.size())) {
             return std::string(buf.begin(), buf.begin() + len);
         }
         buf.resize(buf.size() * 2);

src/util/system.cpp

@@ -1229,7 +1229,7 @@ void RenameThreadPool(ctpl::thread_pool& tp, const char* baseName)
         // `doneCnt` should be at least `futures.size()` if tp size was increased (for whatever reason),
         // or at least `tp.size()` if tp size was decreased and queue was cleared
         // (which can happen on `stop()` if we were not fast enough to get all jobs to their threads).
-    } while (doneCnt < futures.size() && doneCnt < tp.size());
+    } while (uint64_t(doneCnt) < futures.size() && doneCnt < tp.size());
 
     cond->notify_all();
 

src/validation.h

@@ -43,7 +43,7 @@ class CScriptCheck;
 class CBlockPolicyEstimator;
 class CTxMemPool;
 class CValidationState;
-class PrecomputedTransactionData;
+struct PrecomputedTransactionData;
 struct ChainTxData;
 
 struct DisconnectedBlockTransactions;

src/validationinterface.h

@@ -31,7 +31,7 @@ enum class MemPoolRemovalReason;
 
 namespace llmq {
     class CChainLockSig;
-    class CInstantSendLock;
+    struct CInstantSendLock;
     class CRecoveredSig;
 } // namespace llmq
 

src/wallet/init.cpp

@@ -255,7 +255,7 @@ void WalletInit::Construct(InitInterfaces& interfaces) const
 void WalletInit::AutoLockMasternodeCollaterals() const
 {
     // we can't do this before DIP3 is fully initialized
-    for (const auto pwallet : GetWallets()) {
+    for (const auto& pwallet : GetWallets()) {
         pwallet->AutoLockMasternodeCollaterals();
     }
 }

src/wallet/rpcdump.cpp

@@ -821,7 +821,7 @@ UniValue importelectrumwallet(const JSONRPCRequest& request)
     file.close();
     pwallet->ShowProgress("", 100); // hide progress dialog in GUI
 
-    const uint32_t tip_height = locked_chain->getHeight().value_or(-1);
+    const int32_t tip_height = locked_chain->getHeight().value_or(std::numeric_limits<int32_t>::max());
 
     // Whether to perform rescan after import
     int nStartHeight = 0;

src/wallet/rpcwallet.cpp

@@ -3855,7 +3855,7 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request)
     // Find all addresses that have the given label
     UniValue ret(UniValue::VOBJ);
     std::set<std::string> addresses;
-    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
+    for (const std::pair<CTxDestination, CAddressBookData> item : pwallet->mapAddressBook) {
         if (item.second.name == label) {
             std::string address = EncodeDestination(item.first);
             // CWallet::mapAddressBook is not expected to contain duplicate
@@ -3921,7 +3921,7 @@ static UniValue listlabels(const JSONRPCRequest& request)
 
     // Add to a set to sort by label name, then insert into Univalue array
     std::set<std::string> label_set;
-    for (const std::pair<CTxDestination, CAddressBookData>& entry : pwallet->mapAddressBook) {
+    for (const std::pair<CTxDestination, CAddressBookData> entry : pwallet->mapAddressBook) {
         if (purpose.empty() || entry.second.purpose == purpose) {
             label_set.insert(entry.second.name);
         }

src/wallet/test/coinjoin_tests.cpp

@@ -97,7 +97,7 @@ public:
             BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, state));
             AddTxToChain(tx->GetHash());
             for (size_t n = 0; n < tx->vout.size(); ++n) {
-                if (nChangePosRet != -1 && n == nChangePosRet) {
+                if (nChangePosRet != -1 && int(n) == nChangePosRet) {
                     // Skip the change output to only return the requested coins
                     continue;
                 }

src/wallet/test/wallet_tests.cpp

@@ -598,7 +598,7 @@ public:
         std::vector<COutPoint> vecOutpoints;
         size_t n;
         for (n = 0; n < tx->vout.size(); ++n) {
-            if (nChangePosRet != -1 && n == nChangePosRet) {
+            if (nChangePosRet != -1 && int(n) == nChangePosRet) {
                 // Skip the change output to only return the requested coins
                 continue;
             }
@@ -632,7 +632,7 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup)
             {13, {{100001, true}}}
         };
         assert(mapTestCases.size() == mapExpected.size());
-        for (int i = 0; i < mapTestCases.size(); ++i) {
+        for (size_t i = 0; i < mapTestCases.size(); ++i) {
             if (!CreateTransaction(mapTestCases.at(i), mapExpected.at(i).first, mapExpected.at(i).second)) {
                 std::cout << strprintf("CreateTransactionTest failed at: %d - %d\n", nTestId, i) << std::endl;
             }
@@ -858,10 +858,10 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup)
         // Just to create nCount output recipes to use in tests below
         std::vector<std::pair<CAmount, bool>> vecOutputEntries{{5000, false}};
         auto createOutputEntries = [&](int nCount) {
-            while (vecOutputEntries.size() <= nCount) {
+            while (vecOutputEntries.size() <= size_t(nCount)) {
                 vecOutputEntries.push_back(vecOutputEntries.back());
             }
-            if (vecOutputEntries.size() > nCount) {
+            if (vecOutputEntries.size() > size_t(nCount)) {
                 int nDiff = vecOutputEntries.size() - nCount;
                 vecOutputEntries.erase(vecOutputEntries.begin(), vecOutputEntries.begin() + nDiff);
             }
@@ -904,13 +904,15 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup
     // Check initial balance from one mature coinbase transaction.
     BOOST_CHECK_EQUAL(wallet->GetAvailableBalance(), 500 * COIN);
 
-    std::vector<CompactTallyItem> vecTally;
+    {
-    BOOST_CHECK(wallet->SelectCoinsGroupedByAddresses(vecTally, false /*fSkipDenominated*/, false /*fAnonymizable*/,
+        std::vector<CompactTallyItem> vecTally = wallet->SelectCoinsGroupedByAddresses(/*fSkipDenominated=*/false,
-                                                                false /*fSkipUnconfirmed*/, 100/*nMaxOupointsPerAddress*/));
+                /*fAnonymizable=*/false,
+                /*fSkipUnconfirmed=*/false,
+                /*nMaxOupointsPerAddress=*/100);
         BOOST_CHECK_EQUAL(vecTally.size(), 1);
         BOOST_CHECK_EQUAL(vecTally.at(0).nAmount, 500 * COIN);
         BOOST_CHECK_EQUAL(vecTally.at(0).vecInputCoins.size(), 1);
-    vecTally.clear();
+    }
 
     // Create two conflicting transactions, add one to the wallet and mine the other one.
     CTransactionRef tx1;
@@ -946,14 +948,15 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup
 
     // Committed tx is the one that should be marked as "conflicting".
     // Make sure that available balance and SelectCoinsGroupedByAddresses results match.
-    BOOST_CHECK(wallet->SelectCoinsGroupedByAddresses(vecTally, false /*fSkipDenominated*/, false /*fAnonymizable*/,
+    const auto vecTally = wallet->SelectCoinsGroupedByAddresses(/*fSkipDenominated=*/false,
-                                                                false /*fSkipUnconfirmed*/, 100/*nMaxOupointsPerAddress*/));
+            /*fAnonymizable=*/false,
+            /*fSkipUnconfirmed=*/false,
+            /*nMaxOupointsPerAddress=*/100);
     BOOST_CHECK_EQUAL(vecTally.size(), 2);
     BOOST_CHECK_EQUAL(vecTally.at(0).vecInputCoins.size(), 1);
     BOOST_CHECK_EQUAL(vecTally.at(1).vecInputCoins.size(), 1);
     BOOST_CHECK_EQUAL(vecTally.at(0).nAmount + vecTally.at(1).nAmount, (500 + 499) * COIN);
     BOOST_CHECK_EQUAL(wallet->GetAvailableBalance(), (500 + 499) * COIN);
-    vecTally.clear();
 }
 
 BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup)

src/wallet/wallet.cpp

@@ -2773,8 +2773,8 @@ CAmount CWallet::GetAnonymizableBalance(bool fSkipDenominated, bool fSkipUnconfi
 {
     if (!CCoinJoinClientOptions::IsEnabled()) return 0;
 
-    std::vector<CompactTallyItem> vecTally;
+    std::vector<CompactTallyItem> vecTally = SelectCoinsGroupedByAddresses(fSkipDenominated, true, fSkipUnconfirmed);
-    if(!SelectCoinsGroupedByAddresses(vecTally, fSkipDenominated, true, fSkipUnconfirmed)) return 0;
+    if (vecTally.empty()) return 0;
 
     CAmount nTotal = 0;
 
@@ -3351,7 +3351,7 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, interface
     return locktime;
 }
 
-bool CWallet::SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTallyRet, bool fSkipDenominated, bool fAnonymizable, bool fSkipUnconfirmed, int nMaxOupointsPerAddress) const
+std::vector<CompactTallyItem> CWallet::SelectCoinsGroupedByAddresses(bool fSkipDenominated, bool fAnonymizable, bool fSkipUnconfirmed, int nMaxOupointsPerAddress) const
 {
     auto locked_chain = chain().lock();
     LOCK(cs_wallet);
@@ -3362,14 +3362,12 @@ bool CWallet::SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTa
     // This should only be used if nMaxOupointsPerAddress was NOT specified.
     if(nMaxOupointsPerAddress == -1 && fAnonymizable && fSkipUnconfirmed) {
         if(fSkipDenominated && fAnonymizableTallyCachedNonDenom) {
-            vecTallyRet = vecAnonymizableTallyCachedNonDenom;
+            LogPrint(BCLog::SELECTCOINS, "SelectCoinsGroupedByAddresses - using cache for non-denom inputs %d\n", vecAnonymizableTallyCachedNonDenom.size());
-            LogPrint(BCLog::SELECTCOINS, "SelectCoinsGroupedByAddresses - using cache for non-denom inputs %d\n", vecTallyRet.size());
+            return vecAnonymizableTallyCachedNonDenom;
-            return vecTallyRet.size() > 0;
         }
         if(!fSkipDenominated && fAnonymizableTallyCached) {
-            vecTallyRet = vecAnonymizableTallyCached;
+            LogPrint(BCLog::SELECTCOINS, "SelectCoinsGroupedByAddresses - using cache for all inputs %d\n", vecAnonymizableTallyCached.size());
-            LogPrint(BCLog::SELECTCOINS, "SelectCoinsGroupedByAddresses - using cache for all inputs %d\n", vecTallyRet.size());
+            return vecAnonymizableTallyCached;
-            return vecTallyRet.size() > 0;
         }
     }
 
@@ -3399,7 +3397,7 @@ bool CWallet::SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTa
             if(!(mine & filter)) continue;
 
             auto itTallyItem = mapTally.find(txdest);
-            if (nMaxOupointsPerAddress != -1 && itTallyItem != mapTally.end() && itTallyItem->second.vecInputCoins.size() >= nMaxOupointsPerAddress) continue;
+            if (nMaxOupointsPerAddress != -1 && itTallyItem != mapTally.end() && int64_t(itTallyItem->second.vecInputCoins.size()) >= nMaxOupointsPerAddress) continue;
 
             if(IsSpent(*locked_chain, outpoint.hash, i) || IsLockedCoin(outpoint.hash, i)) continue;
 
@@ -3427,7 +3425,7 @@ bool CWallet::SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTa
 
     // construct resulting vector
     // NOTE: vecTallyRet is "sorted" by txdest (i.e. address), just like mapTally
-    vecTallyRet.clear();
+    std::vector<CompactTallyItem> vecTallyRet;
     for (const auto& item : mapTally) {
         if(fAnonymizable && item.second.nAmount < nSmallestDenom) continue;
         vecTallyRet.push_back(item.second);
@@ -3453,7 +3451,7 @@ bool CWallet::SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTa
         LogPrint(BCLog::SELECTCOINS, "%s", strMessage); /* Continued */
     }
 
-    return vecTallyRet.size() > 0;
+    return vecTallyRet;
 }
 
 bool CWallet::SelectDenominatedAmounts(CAmount nValueMax, std::set<CAmount>& setAmountsRet) const

src/wallet/wallet.h

@@ -112,12 +112,9 @@ enum WalletFeature
 struct CompactTallyItem
 {
     CTxDestination txdest;
-    CAmount nAmount;
+    CAmount nAmount{0};
     std::vector<CInputCoin> vecInputCoins;
-    CompactTallyItem()
+    CompactTallyItem() = default;
-    {
-        nAmount = 0;
-    }
 };
 
 enum WalletFlags : uint64_t {
@@ -825,7 +822,7 @@ public:
     bool SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet);
     bool SelectDenominatedAmounts(CAmount nValueMax, std::set<CAmount>& setAmountsRet) const;
 
-    bool SelectCoinsGroupedByAddresses(std::vector<CompactTallyItem>& vecTallyRet, bool fSkipDenominated = true, bool fAnonymizable = true, bool fSkipUnconfirmed = true, int nMaxOupointsPerAddress = -1) const;
+    std::vector<CompactTallyItem> SelectCoinsGroupedByAddresses(bool fSkipDenominated = true, bool fAnonymizable = true, bool fSkipUnconfirmed = true, int nMaxOupointsPerAddress = -1) const;
 
     bool HasCollateralInputs(bool fOnlyConfirmed = true) const;
     int  CountInputsWithAmount(CAmount nInputAmount) const;

src/zmq/zmqabstractnotifier.h

@@ -20,7 +20,7 @@ typedef std::shared_ptr<const CTransaction> CTransactionRef;
 
 namespace llmq {
     class CChainLockSig;
-    class CInstantSendLock;
+    struct CInstantSendLock;
     class CRecoveredSig;
 } // namespace llmq