ci: Add --enable-werror to arm and c++17 builds (and fix all issues found via these builds)

ci/matrix.sh

@@ -40,7 +40,7 @@ if [ "$BUILD_TARGET" = "arm-linux" ]; then
   export CHECK_DOC=1
   # -Wno-psabi is to disable ABI warnings: "note: parameter passing for argument of type ... changed in GCC 7.1"
   # This could be removed once the ABI change warning does not show up by default
-  export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CXXFLAGS=-Wno-psabi"
+  export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports --enable-werror CXXFLAGS=-Wno-psabi"
   export RUN_UNITTESTS=false
   export RUN_INTEGRATIONTESTS=false
 elif [ "$BUILD_TARGET" = "win32" ]; then
@@ -67,7 +67,7 @@ elif [ "$BUILD_TARGET" = "linux64" ]; then
 elif [ "$BUILD_TARGET" = "linux64_cxx17" ]; then
   export HOST=x86_64-unknown-linux-gnu
   export DEP_OPTS="NO_UPNP=1 DEBUG=1"
-  export BITCOIN_CONFIG="--enable-zmq --enable-glibc-back-compat --enable-reduce-exports --enable-crash-hooks --enable-c++17 --with-sanitizers=undefined"
+  export BITCOIN_CONFIG="--enable-zmq --enable-glibc-back-compat --enable-reduce-exports --enable-crash-hooks --enable-c++17 --enable-werror --with-sanitizers=undefined"
   export CPPFLAGS="-DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG"
   export PYZMQ=true
   export RUN_INTEGRATIONTESTS=false

src/bench/bench_dash.cpp

@@ -20,7 +20,6 @@
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 
-static const int64_t DEFAULT_BENCH_EVALUATIONS = 5;
 static const char* DEFAULT_BENCH_FILTER = ".*";
 
 void InitBLSTests();

src/bench/bls_dkg.cpp

@@ -86,8 +86,6 @@ class DKG
         ReceiveVvecs();
         size_t memberIdx = 0;
         bench.minEpochIterations(epoch_iters).run([&] {
-            auto& m = members[memberIdx];
-
             ReceiveShares(memberIdx);
 
             std::set<size_t> invalidIndexes;

src/bench/checkqueue.cpp

@@ -11,7 +11,6 @@
 #include <random.h>
 
 
-static const int MIN_CORES = 2;
 static const size_t BATCHES = 101;
 static const size_t BATCH_SIZE = 30;
 static const int PREVECTOR_SIZE = 28;

src/hdchain.h

@@ -74,6 +74,7 @@ class CHDChain
 
         // by swapping the members of two classes,
         // the two classes are effectively swapped
+        LOCK2(first.cs, second.cs);
         swap(first.nVersion, second.nVersion);
         swap(first.id, second.id);
         swap(first.fCrypted, second.fCrypted);

src/init.cpp

@@ -2046,7 +2046,6 @@ bool AppInitMain()
     LogPrintf("* Using %.1fMiB for in-memory UTXO set (plus up to %.1fMiB of unused mempool space)\n", nCoinCacheUsage * (1.0 / 1024 / 1024), nMempoolSizeMax * (1.0 / 1024 / 1024));
 
     bool fLoaded = false;
-    int64_t nStart = GetTimeMillis();
 
     while (!fLoaded && !ShutdownRequested()) {
         bool fReset = fReindex;

src/llmq/quorums_chainlocks.cpp

@@ -555,23 +555,24 @@ void CChainLocksHandler::EnforceBestChainLock()
         activateNeeded = chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex;
     }
 
-    if (activateNeeded && !ActivateBestChain(state, params)) {
-        LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, FormatStateMessage(state));
-    }
-
-    const CBlockIndex* pindexNotify = nullptr;
-    {
+    if (activateNeeded) {
+        if(!ActivateBestChain(state, params)) {
+            LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, FormatStateMessage(state));
+            return;
+        }
         LOCK(cs_main);
-        if (lastNotifyChainLockBlockIndex != currentBestChainLockBlockIndex &&
-            chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) == currentBestChainLockBlockIndex) {
-            lastNotifyChainLockBlockIndex = currentBestChainLockBlockIndex;
-            pindexNotify = currentBestChainLockBlockIndex;
+        if (chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex) {
+            return;
         }
     }
 
-    if (pindexNotify) {
-        GetMainSignals().NotifyChainLock(pindexNotify, clsig);
+    {
+        LOCK(cs);
+        if (lastNotifyChainLockBlockIndex == currentBestChainLockBlockIndex) return;
+        lastNotifyChainLockBlockIndex = currentBestChainLockBlockIndex;
     }
+
+    GetMainSignals().NotifyChainLock(currentBestChainLockBlockIndex, clsig);
 }
 
 void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)

src/llmq/quorums_dkgsession.cpp

@@ -657,6 +657,8 @@ void CDKGSession::VerifyAndJustify(CDKGPendingMessages& pendingMessages)
 
     std::set<uint256> justifyFor;
 
+    LOCK(invCs);
+
     for (const auto& m : members) {
         if (m->bad) {
             continue;
@@ -1212,21 +1214,25 @@ std::vector<CFinalCommitment> CDKGSession::FinalizeCommitments()
     typedef std::vector<bool> Key;
     std::map<Key, std::vector<CDKGPrematureCommitment>> commitmentsMap;
 
-    for (const auto& p : prematureCommitments) {
-        auto& qc = p.second;
-        if (!validCommitments.count(p.first)) {
-            continue;
-        }
+    {
+        LOCK(invCs);
 
-        // should have been verified before
-        assert(qc.CountValidMembers() >= params.minSize);
+        for (const auto& p : prematureCommitments) {
+            auto& qc = p.second;
+            if (!validCommitments.count(p.first)) {
+                continue;
+            }
 
-        auto it = commitmentsMap.find(qc.validMembers);
-        if (it == commitmentsMap.end()) {
-            it = commitmentsMap.emplace(qc.validMembers, std::vector<CDKGPrematureCommitment>()).first;
-        }
+            // should have been verified before
+            assert(qc.CountValidMembers() >= params.minSize);
 
-        it->second.emplace_back(qc);
+            auto it = commitmentsMap.find(qc.validMembers);
+            if (it == commitmentsMap.end()) {
+                it = commitmentsMap.emplace(qc.validMembers, std::vector<CDKGPrematureCommitment>()).first;
+            }
+
+            it->second.emplace_back(qc);
+        }
     }
 
     std::vector<CFinalCommitment> finalCommitments;

src/llmq/quorums_dkgsessionhandler.cpp

@@ -386,7 +386,7 @@ std::set<NodeId> BatchVerifyMessageSigs(CDKGSession& session, const std::vector<
         }
 
         // are all messages from the same node?
-        NodeId firstNodeId;
+        NodeId firstNodeId{-1};
         first = true;
         bool nodeIdsAllSame = true;
         for (auto it = messages.begin(); it != messages.end(); ++it) {

src/llmq/quorums_dkgsessionhandler.h

@@ -117,10 +117,11 @@ class CDKGSessionHandler
     std::shared_ptr<CDKGSession> curSession;
     std::thread phaseHandlerThread;
 
-    CDKGPendingMessages pendingContributions GUARDED_BY(cs);
-    CDKGPendingMessages pendingComplaints GUARDED_BY(cs);
-    CDKGPendingMessages pendingJustifications GUARDED_BY(cs);
-    CDKGPendingMessages pendingPrematureCommitments GUARDED_BY(cs);
+    // Do not guard these, they protect their internals themselves
+    CDKGPendingMessages pendingContributions;
+    CDKGPendingMessages pendingComplaints;
+    CDKGPendingMessages pendingJustifications;
+    CDKGPendingMessages pendingPrematureCommitments;
 
 public:
     CDKGSessionHandler(const Consensus::LLMQParams& _params, CBLSWorker& blsWorker, CDKGSessionManager& _dkgManager);

src/llmq/quorums_instantsend.cpp

@@ -1418,7 +1418,7 @@ void CInstantSendManager::AskNodesForLockedTx(const uint256& txid)
 {
     std::vector<CNode*> nodesToAskFor;
     g_connman->ForEachNode([&](CNode* pnode) {
-        LOCK(pnode->cs_filter);
+        LOCK(pnode->cs_inventory);
         if (pnode->filterInventoryKnown.contains(txid)) {
             pnode->AddRef();
             nodesToAskFor.emplace_back(pnode);

src/llmq/quorums_signing_shares.cpp

@@ -599,6 +599,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
             }
             auto& sigShare = *ns.pendingIncomingSigShares.GetFirst();
 
+            AssertLockHeld(cs);
             bool alreadyHave = this->sigShares.Has(sigShare.GetKey());
             if (!alreadyHave) {
                 uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash());
@@ -1068,6 +1069,7 @@ void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map<NodeId, st
     std::unordered_map<std::pair<Consensus::LLMQType, uint256>, std::unordered_set<NodeId>, StaticSaltedHasher> quorumNodesMap;
 
     sigSharesQueuedToAnnounce.ForEach([&](const SigShareKey& sigShareKey, bool) {
+        AssertLockHeld(cs);
         auto& signHash = sigShareKey.first;
         auto quorumMember = sigShareKey.second;
         const CSigShare* sigShare = sigShares.Get(sigShareKey);
@@ -1428,6 +1430,7 @@ void CSigSharesManager::Cleanup()
         }
         // remove global requested state to force a re-request from another node
         it->second.requestedSigShares.ForEach([&](const SigShareKey& k, bool) {
+            AssertLockHeld(cs);
             sigSharesRequested.Erase(k);
         });
         nodeStates.erase(nodeId);
@@ -1462,6 +1465,7 @@ void CSigSharesManager::RemoveBannedNodeStates()
         if (IsBanned(it->first)) {
             // re-request sigshares from other nodes
             it->second.requestedSigShares.ForEach([&](const SigShareKey& k, int64_t) {
+                AssertLockHeld(cs);
                 sigSharesRequested.Erase(k);
             });
             it = nodeStates.erase(it);
@@ -1491,6 +1495,7 @@ void CSigSharesManager::BanNode(NodeId nodeId)
 
     // Whatever we requested from him, let's request it from someone else now
     nodeState.requestedSigShares.ForEach([&](const SigShareKey& k, int64_t) {
+        AssertLockHeld(cs);
         sigSharesRequested.Erase(k);
     });
     nodeState.requestedSigShares.Clear();

src/masternode/masternode-sync.cpp

@@ -23,20 +23,24 @@ CMasternodeSync::CMasternodeSync()
 void CMasternodeSync::Reset(bool fForce, bool fNotifyReset)
 {
     // Avoid resetting the sync process if we just "recently" received a new block
-    if (fForce || (GetTime() - nTimeLastUpdateBlockTip > MASTERNODE_SYNC_RESET_SECONDS)) {
-        {
-            LOCK(cs);
-            nCurrentAsset = MASTERNODE_SYNC_BLOCKCHAIN;
-            nTriedPeerCount = 0;
-            nTimeAssetSyncStarted = GetTime();
-            nTimeLastBumped = GetTime();
-            nTimeLastUpdateBlockTip = 0;
-            fReachedBestHeader = false;
-        }
-        if (fNotifyReset) {
-            uiInterface.NotifyAdditionalDataSyncProgressChanged(-1);
+    if (!fForce) {
+        LOCK(cs);
+        if (GetTime() - nTimeLastUpdateBlockTip < MASTERNODE_SYNC_RESET_SECONDS) {
+            return;
         }
     }
+    {
+        LOCK(cs);
+        nCurrentAsset = MASTERNODE_SYNC_BLOCKCHAIN;
+        nTriedPeerCount = 0;
+        nTimeAssetSyncStarted = GetTime();
+        nTimeLastBumped = GetTime();
+        nTimeLastUpdateBlockTip = 0;
+        fReachedBestHeader = false;
+    }
+    if (fNotifyReset) {
+        uiInterface.NotifyAdditionalDataSyncProgressChanged(-1);
+    }
 }
 
 void CMasternodeSync::BumpAssetLastTime(const std::string& strFuncName)
@@ -88,6 +92,7 @@ void CMasternodeSync::SwitchToNextAsset(CConnman& connman)
 
 std::string CMasternodeSync::GetSyncStatus() const
 {
+    LOCK(cs);
     switch (nCurrentAsset) {
         case MASTERNODE_SYNC_BLOCKCHAIN:    return _("Synchronizing blockchain...");
         case MASTERNODE_SYNC_GOVERNANCE:    return _("Synchronizing governance objects...");
@@ -143,6 +148,7 @@ void CMasternodeSync::ProcessTick(CConnman& connman)
         return;
     }
 
+    LOCK(cs);
     // Calculate "progress" for LOG reporting / GUI notification
     double nSyncProgress = double(nTriedPeerCount + (nCurrentAsset - 1) * 8) / (8*4);
     LogPrint(BCLog::MNSYNC, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d nTriedPeerCount %d nSyncProgress %f\n", nTick, nCurrentAsset, nTriedPeerCount, nSyncProgress);
@@ -371,17 +377,15 @@ void CMasternodeSync::UpdatedBlockTip(const CBlockIndex *pindexNew, bool fInitia
     // Note: since we sync headers first, it should be ok to use this
     bool fReachedBestHeaderNew = pindexNew->GetBlockHash() == pindexTip->GetBlockHash();
 
+    LOCK(cs);
     if (fReachedBestHeader && !fReachedBestHeaderNew) {
         // Switching from true to false means that we previously stuck syncing headers for some reason,
         // probably initial timeout was not enough,
         // because there is no way we can update tip not having best header
         Reset(true);
     }
 
-    {
-        LOCK(cs);
-        fReachedBestHeader = fReachedBestHeaderNew;
-    }
+    fReachedBestHeader = fReachedBestHeaderNew;
     LogPrint(BCLog::MNSYNC, "CMasternodeSync::UpdatedBlockTip -- pindexNew->nHeight: %d pindexTip->nHeight: %d fInitialDownload=%d fReachedBestHeader=%d\n",
                 pindexNew->nHeight, pindexTip->nHeight, fInitialDownload, fReachedBestHeader);
 }

src/net.cpp

@@ -1278,15 +1278,15 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
     m_msgproc->InitializeNode(pnode);
 
     if (fLogIPs) {
-        LogPrint(BCLog::NET_NETCONN, "connection from %s accepted, sock=%d, peer=%d\n", addr.ToString(), pnode->hSocket, pnode->GetId());
+        LogPrint(BCLog::NET_NETCONN, "connection from %s accepted, sock=%d, peer=%d\n", addr.ToString(), hSocket, pnode->GetId());
     } else {
-        LogPrint(BCLog::NET_NETCONN, "connection accepted, sock=%d, peer=%d\n", pnode->hSocket, pnode->GetId());
+        LogPrint(BCLog::NET_NETCONN, "connection accepted, sock=%d, peer=%d\n", hSocket, pnode->GetId());
     }
 
     {
         LOCK(cs_vNodes);
         vNodes.push_back(pnode);
-        mapSocketToNode.emplace(pnode->hSocket, pnode);
+        mapSocketToNode.emplace(hSocket, pnode);
         RegisterEvents(pnode);
         WakeSelect();
     }
@@ -1443,8 +1443,9 @@ void CConnman::CalculateNumConnectionsChangedStats()
     }
     mapRecvBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0;
     mapSentBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0;
-    LOCK(cs_vNodes);
-    for (const CNode* pnode : vNodes) {
+    auto vNodesCopy = CopyNodeVector(CConnman::FullyConnectedOnly);
+    for (auto pnode : vNodesCopy) {
+        LOCK(pnode->cs_vRecv);
         for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd)
             mapRecvBytesMsgStats[i.first] += i.second;
         for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd)
@@ -1466,6 +1467,7 @@ void CConnman::CalculateNumConnectionsChangedStats()
         if(pnode->nPingUsecTime > 0)
             statsClient.timing("peers.ping_us", pnode->nPingUsecTime, 1.0f);
     }
+    ReleaseNodeVector(vNodesCopy);
     for (const std::string &msg : getAllNetMessageTypes()) {
         statsClient.gauge("bandwidth.message." + msg + ".totalBytesReceived", mapRecvBytesMsgStats[msg], 1.0f);
         statsClient.gauge("bandwidth.message." + msg + ".totalBytesSent", mapSentBytesMsgStats[msg], 1.0f);
@@ -2787,7 +2789,12 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
         LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- ConnectNode failed for %s\n", __func__, getIpStr());
         return;
     }
-    LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- succesfully connected to %s, sock=%d, peer=%d\n", __func__, getIpStr(), pnode->hSocket, pnode->GetId());
+
+    {
+        LOCK(pnode->cs_hSocket);
+        LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- succesfully connected to %s, sock=%d, peer=%d\n", __func__, getIpStr(), pnode->hSocket, pnode->GetId());
+    }
+
     if (grantOutbound)
         grantOutbound->MoveTo(pnode->grantOutbound);
     if (fOneShot)
@@ -2802,7 +2809,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
         pnode->m_masternode_probe_connection = true;
 
     {
-        LOCK(cs_vNodes);
+        LOCK2(cs_vNodes, pnode->cs_hSocket);
         mapSocketToNode.emplace(pnode->hSocket, pnode);
     }
 
@@ -3374,7 +3381,10 @@ void CConnman::Stop()
     }
     vNodes.clear();
     mapSocketToNode.clear();
-    mapReceivableNodes.clear();
+    {
+        LOCK(cs_vNodes);
+        mapReceivableNodes.clear();
+    }
     {
         LOCK(cs_mapNodesWithDataToSend);
         mapNodesWithDataToSend.clear();

src/net_processing.cpp

@@ -1077,7 +1077,6 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV
     }
 }
 
-// Requires cs_main.
 bool IsBanned(NodeId pnode)
 {
     CNodeState *state = State(pnode);
@@ -4227,13 +4226,13 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         //
         std::vector<CInv> vInv;
         {
+            LOCK2(mempool.cs, pto->cs_inventory);
+
             size_t reserve = std::min<size_t>(pto->setInventoryTxToSend.size(), INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000);
             reserve = std::max<size_t>(reserve, pto->vInventoryBlockToSend.size());
             reserve = std::min<size_t>(reserve, MAX_INV_SZ);
             vInv.reserve(reserve);
 
-            LOCK2(mempool.cs, pto->cs_inventory);
-
             // Add blocks
             for (const uint256& hash : pto->vInventoryBlockToSend) {
                 vInv.push_back(CInv(MSG_BLOCK, hash));