From b797fbd827fb61c991f004d89bd65aa0a087ded6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 18 Jun 2021 01:15:16 +0300 Subject: [PATCH 01/17] ci: Add `--enable-werror` to arm and c++17 builds Kind of similar to changes in 18145 --- ci/matrix.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/matrix.sh b/ci/matrix.sh index be6b92516f40..db43046a25e0 100755 --- a/ci/matrix.sh +++ b/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 From a18707e397c878e7295d9af1c13879da2532557d Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 10:52:50 +0300 Subject: [PATCH 02/17] Remove/fix unused variables --- src/bench/bench_dash.cpp | 1 - src/bench/bls_dkg.cpp | 2 -- src/bench/checkqueue.cpp | 1 - src/init.cpp | 1 - src/policy/fees.cpp | 2 +- src/qt/coincontroldialog.cpp | 1 - src/qt/networkstyle.cpp | 5 ++--- src/qt/walletframe.cpp | 2 +- src/support/allocators/mt_pooled_secure.h | 1 - src/test/evo_deterministicmns_tests.cpp | 1 - src/validation.cpp | 5 +---- src/wallet/wallet.cpp | 2 -- 12 files changed, 5 insertions(+), 19 deletions(-) diff --git a/src/bench/bench_dash.cpp b/src/bench/bench_dash.cpp index bcaf85d5d1ac..8fe2ddb0e2ac 100644 --- a/src/bench/bench_dash.cpp +++ b/src/bench/bench_dash.cpp @@ -20,7 +20,6 @@ const std::function G_TRANSLATION_FUN = nullptr; -static const int64_t DEFAULT_BENCH_EVALUATIONS = 5; static const char* DEFAULT_BENCH_FILTER = ".*"; void InitBLSTests(); diff --git a/src/bench/bls_dkg.cpp b/src/bench/bls_dkg.cpp index a10145297970..07860b9516db 100644 --- a/src/bench/bls_dkg.cpp +++ b/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 invalidIndexes; diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index 262cab96ebdb..97f799fa9ddf 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -11,7 +11,6 @@ #include -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; diff --git a/src/init.cpp b/src/init.cpp index 7a923d7e9468..7d3307681280 100644 --- a/src/init.cpp +++ b/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; diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index aa0daa512240..1a3a42c0ead8 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -925,7 +925,7 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein) try { LOCK(cs_feeEstimator); int nVersionRequired, nVersionThatWrote; - unsigned int nFileBestSeenHeight, nFileHistoricalFirst, nFileHistoricalBest; + unsigned int nFileBestSeenHeight; filein >> nVersionRequired >> nVersionThatWrote; if (nVersionRequired > CLIENT_VERSION) return error("CBlockPolicyEstimator::Read(): up-version (%d) fee estimate file", nVersionRequired); diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 3bbc26033a8e..d6a931d68ada 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -481,7 +481,6 @@ void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel * unsigned int nBytes = 0; unsigned int nBytesInputs = 0; unsigned int nQuantity = 0; - int nQuantityUncompressed = 0; bool fUnselectedSpent{false}; bool fUnselectedNonMixed{false}; diff --git a/src/qt/networkstyle.cpp b/src/qt/networkstyle.cpp index 639edda518b7..7a9c277e2c8a 100644 --- a/src/qt/networkstyle.cpp +++ b/src/qt/networkstyle.cpp @@ -42,9 +42,8 @@ void NetworkStyle::rotateColor(QColor& col, const int iconColorHueShift, const i col.setHsl(h,s,l,a); } -void NetworkStyle::rotateColors(QImage& img, const int iconColorHueShift, const int iconColorSaturationReduction) { - int h,s,l,a; - +void NetworkStyle::rotateColors(QImage& img, const int iconColorHueShift, const int iconColorSaturationReduction) +{ // traverse though lines for(int y=0;y::const_iterator i; - for (auto i = mapWalletViews.constBegin(); i != mapWalletViews.constEnd(); ++i) { + for (i = mapWalletViews.constBegin(); i != mapWalletViews.constEnd(); ++i) { i.value()->gotoCoinJoinCoinsPage(addr); } } diff --git a/src/support/allocators/mt_pooled_secure.h b/src/support/allocators/mt_pooled_secure.h index f6f891096c19..38bd003e2ce8 100644 --- a/src/support/allocators/mt_pooled_secure.h +++ b/src/support/allocators/mt_pooled_secure.h @@ -63,7 +63,6 @@ struct mt_pooled_secure_allocator : public std::allocator { private: size_t get_bucket() { - auto tid = std::this_thread::get_id(); size_t x = std::hash{}(std::this_thread::get_id()); return x % pools.size(); } diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index a96824db2a0d..4d89c295415c 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -517,7 +517,6 @@ BOOST_FIXTURE_TEST_CASE(dip3_test_mempool_reorg, TestChainDIP3Setup) BOOST_FIXTURE_TEST_CASE(dip3_test_mempool_dual_proregtx, TestChainDIP3Setup) { - int nHeight = chainActive.Height(); auto utxos = BuildSimpleUtxoMap(m_coinbase_txns); // Create a MN diff --git a/src/validation.cpp b/src/validation.cpp index 12223e8d40ad..f8e947c03ffb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -693,8 +693,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool auto itConflicting = pool.mapNextTx.find(txin.prevout); if (itConflicting != pool.mapNextTx.end()) { - const CTransaction *ptxConflicting = itConflicting->second; - // Transaction conflicts with mempool and RBF doesn't exist in Dash return state.Invalid(false, REJECT_DUPLICATE, "txn-mempool-conflict"); } @@ -824,7 +822,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Check against previous transactions // This is done last to help prevent CPU exhaustion denial-of-service attacks. PrecomputedTransactionData txdata(tx); - if (!CheckInputs(tx, state, view, true, STANDARD_SCRIPT_VERIFY_FLAGS, true, false, txdata)) + if (!CheckInputs(tx, state, view, true, scriptVerifyFlags, true, false, txdata)) return false; // state filled in by CheckInputs // Check again against the current block tip's script verification @@ -1235,7 +1233,6 @@ bool IsInitialBlockDownload() return false; if (fImporting || fReindex) return true; - const CChainParams& chainParams = Params(); if (chainActive.Tip() == nullptr) return true; if (chainActive.Tip()->nChainWork < nMinimumChainWork) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7a8766adef3d..415da8d2e42f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5530,8 +5530,6 @@ void CMerkleTx::SetMerkleBranch(const CBlockIndex* pindex, int posInBlock) int CMerkleTx::GetDepthInMainChain() const { - int nResult; - if (hashUnset()) return 0; From 0f17a13b9b12ecf5708ba3c378eb4a25584ef5e6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 10:54:27 +0300 Subject: [PATCH 03/17] Remove duplicate rescanblockchain declaration leftover from 13899 --- src/wallet/rpcwallet.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b5b57b6a58c7..026d04f2c443 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4376,7 +4376,6 @@ UniValue importwallet(const JSONRPCRequest& request); UniValue importprunedfunds(const JSONRPCRequest& request); UniValue removeprunedfunds(const JSONRPCRequest& request); UniValue importmulti(const JSONRPCRequest& request); -UniValue rescanblockchain(const JSONRPCRequest& request); UniValue dumphdinfo(const JSONRPCRequest& request); UniValue importelectrumwallet(const JSONRPCRequest& request); From 934826d0d2f20926d42fe6c5541c748bf9a81cb5 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 10:59:57 +0300 Subject: [PATCH 04/17] Hold locks in CHDChain::swap() --- src/hdchain.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/hdchain.h b/src/hdchain.h index d1ee44515180..bb027b025779 100644 --- a/src/hdchain.h +++ b/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); From d2a1bab978f671d7df883f84e431b44bba0a009b Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:01:37 +0300 Subject: [PATCH 05/17] Fix locks in EnforceBestChainLock() --- src/llmq/quorums_chainlocks.cpp | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/llmq/quorums_chainlocks.cpp b/src/llmq/quorums_chainlocks.cpp index 1e9b192a94ad..fffaef4d91d3 100644 --- a/src/llmq/quorums_chainlocks.cpp +++ b/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) From 3e95d592d8f3436e98661481f86cc99ce1309ca0 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:05:13 +0300 Subject: [PATCH 06/17] Fix locks in CDKGSession --- src/llmq/quorums_dkgsession.cpp | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/llmq/quorums_dkgsession.cpp b/src/llmq/quorums_dkgsession.cpp index 360522f6b7c3..f001ace91391 100644 --- a/src/llmq/quorums_dkgsession.cpp +++ b/src/llmq/quorums_dkgsession.cpp @@ -657,6 +657,8 @@ void CDKGSession::VerifyAndJustify(CDKGPendingMessages& pendingMessages) std::set justifyFor; + LOCK(invCs); + for (const auto& m : members) { if (m->bad) { continue; @@ -1212,21 +1214,25 @@ std::vector CDKGSession::FinalizeCommitments() typedef std::vector Key; std::map> 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()).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()).first; + } + + it->second.emplace_back(qc); + } } std::vector finalCommitments; From a65a4669a7f220dad9396a78025827ca5e2ef565 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:05:53 +0300 Subject: [PATCH 07/17] Drop external protection of pending messages in CDKGSessionHandler --- src/llmq/quorums_dkgsessionhandler.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/llmq/quorums_dkgsessionhandler.h b/src/llmq/quorums_dkgsessionhandler.h index abbefae5d638..121b7dd13f68 100644 --- a/src/llmq/quorums_dkgsessionhandler.h +++ b/src/llmq/quorums_dkgsessionhandler.h @@ -117,10 +117,11 @@ class CDKGSessionHandler std::shared_ptr 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); From cf3896304a8d3b9e784b862495b6c12410dec33d Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:06:57 +0300 Subject: [PATCH 08/17] Hold the right lock in AskNodesForLockedTx --- src/llmq/quorums_instantsend.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index 7e5d8117d855..9083761dcd30 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -1418,7 +1418,7 @@ void CInstantSendManager::AskNodesForLockedTx(const uint256& txid) { std::vector 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); From 66f3f4f328ba6e9d7409e1ec96b105fc5713e4bd Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:07:45 +0300 Subject: [PATCH 09/17] Check for locks in more places in CSigSharesManager --- src/llmq/quorums_signing_shares.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp index d9c45a311441..4e7671e1ac56 100644 --- a/src/llmq/quorums_signing_shares.cpp +++ b/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, std::unordered_set, 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(); From cc601e80c50e1959cf5e09a73f0aeecf26f58ee6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:10:42 +0300 Subject: [PATCH 10/17] Fix CConnman related locks --- src/net.cpp | 26 ++++++++++++++++++-------- src/net_processing.cpp | 4 ++-- src/test/test_dash.cpp | 2 +- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 0be9499d5c28..6f66f9d36243 100644 --- a/src/net.cpp +++ b/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(); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 300d64038993..4cb940e6603c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4227,13 +4227,13 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // std::vector vInv; { + LOCK2(mempool.cs, pto->cs_inventory); + size_t reserve = std::min(pto->setInventoryTxToSend.size(), INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000); reserve = std::max(reserve, pto->vInventoryBlockToSend.size()); reserve = std::min(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)); diff --git a/src/test/test_dash.cpp b/src/test/test_dash.cpp index cfc240198752..3fcf2e4e20f5 100644 --- a/src/test/test_dash.cpp +++ b/src/test/test_dash.cpp @@ -31,7 +31,7 @@ const std::function G_TRANSLATION_FUN = nullptr; void CConnmanTest::AddNode(CNode& node) { - LOCK(g_connman->cs_vNodes); + LOCK2(g_connman->cs_vNodes, node.cs_hSocket); g_connman->vNodes.push_back(&node); g_connman->mapSocketToNode.emplace(node.hSocket, &node); } From 64d6e5720f7d7bd24548f5471bc17d847bd5059f Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:11:33 +0300 Subject: [PATCH 11/17] Fix locks in CMasternodeSync --- src/masternode/masternode-sync.cpp | 36 +++++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/masternode/masternode-sync.cpp b/src/masternode/masternode-sync.cpp index d236376782c0..bb962b646116 100644 --- a/src/masternode/masternode-sync.cpp +++ b/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,6 +377,7 @@ 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, @@ -378,10 +385,7 @@ void CMasternodeSync::UpdatedBlockTip(const CBlockIndex *pindexNew, bool fInitia 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); } From 8934a653e79f1b4aab3134cd3ef1680c6d700f6a Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:12:11 +0300 Subject: [PATCH 12/17] Fix mempool locks --- src/test/miner_tests.cpp | 1 + src/txmempool.cpp | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 214ae6d12605..86b4dc4ed3d6 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -531,6 +531,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) SetMockTime(0); mempool.clear(); + LOCK(::mempool.cs); TestPackageSelection(chainparams, scriptPubKey, txFirst); fCheckpointsEnabled = true; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 2908765d1c38..6f811c0b96dd 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -860,6 +860,7 @@ void CTxMemPool::removeProTxSpentCollateralConflicts(const CTransaction &tx) // Remove TXs that refer to a MN for which the collateral was spent auto removeSpentCollateralConflict = [&](const uint256& proTxHash) { // Can't use equal_range here as every call to removeRecursive might invalidate iterators + AssertLockHeld(cs); while (true) { auto it = mapProTxRefs.find(proTxHash); if (it == mapProTxRefs.end()) { @@ -1248,6 +1249,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { LOCK(cs); auto hasKeyChangeInMempool = [&](const uint256& proTxHash) { + AssertLockHeld(cs); for (auto its = mapProTxRefs.equal_range(proTxHash); its.first != its.second; ++its.first) { auto txit = mapTx.find(its.first->second); if (txit == mapTx.end()) { From 4aa8689a1595ed1a2f7d7f107b2c162bf9c9446a Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:14:43 +0300 Subject: [PATCH 13/17] Remove duplicates/add checks where needed for cs_main/cs_wallet --- src/rpc/rpcevo.cpp | 2 -- src/validation.cpp | 1 - src/wallet/wallet.cpp | 12 +++++------- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/rpc/rpcevo.cpp b/src/rpc/rpcevo.cpp index 1179075d1141..363ee23c9b9f 100644 --- a/src/rpc/rpcevo.cpp +++ b/src/rpc/rpcevo.cpp @@ -981,8 +981,6 @@ static UniValue protx_list(const JSONRPCRequest& request) g_txindex->BlockUntilSyncedToCurrentChain(); } - LOCK(cs_main); - if (type == "wallet") { if (!pwallet) { throw std::runtime_error("\"protx list wallet\" not supported when wallet is disabled"); diff --git a/src/validation.cpp b/src/validation.cpp index f8e947c03ffb..77ae1e365e1d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2350,7 +2350,6 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl } else { // The node which relayed this should switch to correct chain. // TODO: relay instantsend data/proof. - LOCK(cs_main); return state.DoS(10, error("ConnectBlock(DASH): transaction %s conflicts with transaction lock %s", tx->GetHash().ToString(), conflictLock->txid.ToString()), REJECT_INVALID, "conflict-tx-lock"); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 415da8d2e42f..03c9c5bc44f8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2321,6 +2321,7 @@ std::set CWalletTx::GetConflicts() const std::set result; if (pwallet != nullptr) { + AssertLockHeld(pwallet->cs_wallet); uint256 myHash = GetHash(); result = pwallet->GetConflicts(myHash); result.erase(myHash); @@ -4053,13 +4054,10 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet) fFirstRunRet = mapKeys.empty() && mapHdPubKeys.empty() && mapCryptedKeys.empty() && mapWatchKeys.empty() && setWatchOnly.empty() && mapScripts.empty() && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); } - { - LOCK2(cs_main, cs_wallet); - for (auto& pair : mapWallet) { - for(unsigned int i = 0; i < pair.second.tx->vout.size(); ++i) { - if (IsMine(pair.second.tx->vout[i]) && !IsSpent(pair.first, i)) { - setWalletUTXO.insert(COutPoint(pair.first, i)); - } + for (auto& pair : mapWallet) { + for(unsigned int i = 0; i < pair.second.tx->vout.size(); ++i) { + if (IsMine(pair.second.tx->vout[i]) && !IsSpent(pair.first, i)) { + setWalletUTXO.insert(COutPoint(pair.first, i)); } } } From 69a72ea5d5c3352153724dd92b7e15794e4a89c1 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 23 Jun 2021 11:15:12 +0300 Subject: [PATCH 14/17] Initialize firstNodeId --- src/llmq/quorums_dkgsessionhandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/llmq/quorums_dkgsessionhandler.cpp b/src/llmq/quorums_dkgsessionhandler.cpp index 05dcc1889785..f8097b4405a0 100644 --- a/src/llmq/quorums_dkgsessionhandler.cpp +++ b/src/llmq/quorums_dkgsessionhandler.cpp @@ -386,7 +386,7 @@ std::set 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) { From b472a7025a76dc366c2e34fcc57fe95e4f3ded63 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 9 Jul 2021 01:52:39 +0300 Subject: [PATCH 15/17] Initialize nBytes in CreateTransaction --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 03c9c5bc44f8..6d34676dfc83 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3589,7 +3589,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac assert(txNew.nLockTime < LOCKTIME_THRESHOLD); FeeCalculation feeCalc; CFeeRate discard_rate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this, ::feeEstimator); - unsigned int nBytes; + unsigned int nBytes{0}; { std::vector vecCoins; LOCK2(cs_main, mempool.cs); From d1ff298c8e44243ee486f7cb954d7347d3725fdb Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 10 Jul 2021 17:46:33 +0300 Subject: [PATCH 16/17] Add more compile time checking for cs_main runtime locking assertions --- src/net_processing.cpp | 1 - src/net_processing.h | 8 ++++---- src/txmempool.h | 12 ++++++------ src/util/system.cpp | 2 +- src/validation.cpp | 4 ++-- src/validation.h | 2 +- src/wallet/wallet.h | 4 ++-- 7 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4cb940e6603c..001cf9a486f8 100644 --- a/src/net_processing.cpp +++ b/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); diff --git a/src/net_processing.h b/src/net_processing.h index 5e24d53ff98c..b59349b32a67 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -87,15 +87,15 @@ struct CNodeStateStats { /** Get statistics from node state */ bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats); -bool IsBanned(NodeId nodeid); +bool IsBanned(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Upstream moved this into net_processing.cpp (13417), however since we use Misbehaving in a number of dash specific // files such as mnauth.cpp and governance.cpp it makes sense to keep it in the header /** Increase a node's misbehavior score. */ void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="") EXCLUSIVE_LOCKS_REQUIRED(cs_main); -void EraseObjectRequest(NodeId nodeId, const CInv& inv); -void RequestObject(NodeId nodeId, const CInv& inv, std::chrono::microseconds current_time, bool fForce=false); -size_t GetRequestedObjectCount(NodeId nodeId); +void EraseObjectRequest(NodeId nodeId, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +void RequestObject(NodeId nodeId, const CInv& inv, std::chrono::microseconds current_time, bool fForce=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +size_t GetRequestedObjectCount(NodeId nodeId) EXCLUSIVE_LOCKS_REQUIRED(cs_main); #endif // BITCOIN_NET_PROCESSING_H diff --git a/src/txmempool.h b/src/txmempool.h index 2acefb4106c8..b48fa5ba2ad8 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -583,12 +583,12 @@ class CTxMemPool void removeRecursive(const CTransaction &tx, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN); void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void removeConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeProTxPubKeyConflicts(const CTransaction &tx, const CKeyID &keyId); - void removeProTxPubKeyConflicts(const CTransaction &tx, const CBLSPublicKey &pubKey); - void removeProTxCollateralConflicts(const CTransaction &tx, const COutPoint &collateralOutpoint); - void removeProTxSpentCollateralConflicts(const CTransaction &tx); - void removeProTxKeyChangedConflicts(const CTransaction &tx, const uint256& proTxHash, const uint256& newKeyHash); - void removeProTxConflicts(const CTransaction &tx); + void removeProTxPubKeyConflicts(const CTransaction &tx, const CKeyID &keyId) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeProTxPubKeyConflicts(const CTransaction &tx, const CBLSPublicKey &pubKey) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeProTxCollateralConflicts(const CTransaction &tx, const COutPoint &collateralOutpoint) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeProTxSpentCollateralConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeProTxKeyChangedConflicts(const CTransaction &tx, const uint256& proTxHash, const uint256& newKeyHash) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeProTxConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight); void clear(); diff --git a/src/util/system.cpp b/src/util/system.cpp index c3eb1ba361cc..45a6461ab937 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -238,7 +238,7 @@ class ArgsManagerHelper { } /** Convert regular argument into the network-specific setting */ - static inline std::string NetworkArg(const ArgsManager& am, const std::string& arg) + static inline std::string NetworkArg(const ArgsManager& am, const std::string& arg) EXCLUSIVE_LOCKS_REQUIRED(am.cs_args) { assert(arg.length() > 1 && arg[0] == '-'); return "-" + am.m_network + "." + arg.substr(1); diff --git a/src/validation.cpp b/src/validation.cpp index 77ae1e365e1d..62c10d605da9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -197,7 +197,7 @@ class CChainState { bool ReplayBlocks(const CChainParams& params, CCoinsView* view); bool LoadGenesisBlock(const CChainParams& chainparams); - bool AddGenesisBlock(const CChainParams& chainparams, const CBlock& block, CValidationState& state); + bool AddGenesisBlock(const CChainParams& chainparams, const CBlock& block, CValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void PruneBlockIndexCandidates(); @@ -1363,7 +1363,7 @@ void static InvalidChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(c CheckForkWarningConditions(); } -void static ConflictingChainFound(CBlockIndex* pindexNew) +void static ConflictingChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { statsClient.inc("warnings.ConflictingChainFound", 1.0f); diff --git a/src/validation.h b/src/validation.h index f33d56c23c4e..337a5ed66051 100644 --- a/src/validation.h +++ b/src/validation.h @@ -306,7 +306,7 @@ uint64_t CalculateCurrentUsage(); /** * Mark one block file as pruned. */ -void PruneOneBlockFile(const int fileNumber); +void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * Actually unlink the specified files diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ac44ecb7d402..bf77c510a029 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -487,8 +487,8 @@ class CWalletTx : public CMerkleTx CAmount GetImmatureWatchOnlyCredit(const bool fUseCache=true) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); CAmount GetChange() const; - CAmount GetAnonymizedCredit(const CCoinControl* coinControl = nullptr) const; - CAmount GetDenominatedCredit(bool unconfirmed, bool fUseCache=true) const; + CAmount GetAnonymizedCredit(const CCoinControl* coinControl = nullptr) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + CAmount GetDenominatedCredit(bool unconfirmed, bool fUseCache=true) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Get the marginal bytes if spending the specified output from this transaction int GetSpendSize(unsigned int out, bool use_max_sig = false) const From 9047608bb373b0f81f197e6d05da921649af2a32 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 18 Jun 2021 01:15:16 +0300 Subject: [PATCH 17/17] Fix data races triggered by functional tests. Function CWallet::KeepKey requires locking as it has concurrent access to database and member nKeysLeftSinceAutoBackup. Avoid data race when reading setInventoryTxToSend size by locking the read. If locking happens after the read, the size may change. Lock cs_mnauth when reading verifiedProRegTxHash. Make fRPCRunning atomic as it can be read/written from different threads simultaneously. Make m_masternode_iqr_connection atomic as it can be read/written from different threads simultaneously. Use a recursive mutex to synchronize concurrent access to quorumVvec. Make m_masternode_connection atomic as it can be read/written from different threads simultaneously. Make m_masternode_probe_connection atomic as it can be read/written from different threads simultaneously. Use a recursive mutex in order to lock access to activeMasterNode. Use a recursive mutex to synchronize concurrent access to skShare. Guarded all mnauth fields of a CNode. --- src/coinjoin/coinjoin-server.cpp | 10 +-- src/coinjoin/coinjoin.cpp | 4 +- src/evo/mnauth.cpp | 93 +++++++++++-------------- src/init.cpp | 30 +++++--- src/llmq/quorums.cpp | 95 +++++++++++++++----------- src/llmq/quorums.h | 12 ++-- src/llmq/quorums_dkgsession.cpp | 18 ++--- src/llmq/quorums_dkgsessionhandler.cpp | 2 +- src/llmq/quorums_signing.cpp | 4 +- src/llmq/quorums_signing_shares.cpp | 24 ++++--- src/masternode/activemasternode.cpp | 12 ++-- src/masternode/activemasternode.h | 1 + src/net.cpp | 26 ++++--- src/net.h | 61 ++++++++++++++--- src/net_processing.cpp | 15 ++-- src/rpc/governance.cpp | 17 +++-- src/rpc/masternode.cpp | 12 ++-- src/rpc/misc.cpp | 5 +- src/rpc/rpcquorums.cpp | 10 +-- src/rpc/server.cpp | 2 +- src/wallet/wallet.cpp | 13 ++-- 21 files changed, 274 insertions(+), 192 deletions(-) diff --git a/src/coinjoin/coinjoin-server.cpp b/src/coinjoin/coinjoin-server.cpp index a1f03a56ae0f..ad66126db40c 100644 --- a/src/coinjoin/coinjoin-server.cpp +++ b/src/coinjoin/coinjoin-server.cpp @@ -56,7 +56,7 @@ void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand LogPrint(BCLog::COINJOIN, "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CCoinJoin::DenominationToString(dsa.nDenom), dsa.txCollateral.ToString()); /* Continued */ auto mnList = deterministicMNManager->GetListAtChainTip(); - auto dmn = mnList.GetValidMNByCollateral(activeMasternodeInfo.outpoint); + auto dmn = WITH_LOCK(activeMasternodeInfoCs, return mnList.GetValidMNByCollateral(activeMasternodeInfo.outpoint)); if (!dmn) { PushStatus(pfrom, STATUS_REJECTED, ERR_MN_LIST, connman); return; @@ -68,7 +68,7 @@ void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand if (!lockRecv) return; for (const auto& q : vecCoinJoinQueue) { - if (q.masternodeOutpoint == activeMasternodeInfo.outpoint) { + if (WITH_LOCK(activeMasternodeInfoCs, return q.masternodeOutpoint == activeMasternodeInfo.outpoint)) { // refuse to create another queue this often LogPrint(BCLog::COINJOIN, "DSACCEPT -- last dsq is still in queue, refuse to mix\n"); PushStatus(pfrom, STATUS_REJECTED, ERR_RECENT, connman); @@ -334,7 +334,7 @@ void CCoinJoinServer::CommitFinalTransaction(CConnman& connman) // create and sign masternode dstx transaction if (!CCoinJoin::GetDSTX(hashTx)) { - CCoinJoinBroadcastTx dstxNew(finalTransaction, activeMasternodeInfo.outpoint, GetAdjustedTime()); + CCoinJoinBroadcastTx dstxNew(finalTransaction, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime()); dstxNew.Sign(); CCoinJoin::AddDSTX(dstxNew); } @@ -501,7 +501,7 @@ void CCoinJoinServer::CheckForCompleteQueue(CConnman& connman) if (nState == POOL_STATE_QUEUE && IsSessionReady()) { SetState(POOL_STATE_ACCEPTING_ENTRIES); - CCoinJoinQueue dsq(nSessionDenom, activeMasternodeInfo.outpoint, GetAdjustedTime(), true); + CCoinJoinQueue dsq(nSessionDenom, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime(), true); LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckForCompleteQueue -- queue is ready, signing and relaying (%s) " /* Continued */ "with %d participants\n", dsq.ToString(), vecSessionCollaterals.size()); dsq.Sign(); @@ -708,7 +708,7 @@ bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage& if (!fUnitTest) { //broadcast that I'm accepting entries, only if it's the first entry through - CCoinJoinQueue dsq(nSessionDenom, activeMasternodeInfo.outpoint, GetAdjustedTime(), false); + CCoinJoinQueue dsq(nSessionDenom, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime(), false); LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- signing and relaying new queue: %s\n", dsq.ToString()); dsq.Sign(); dsq.Relay(connman); diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp index 6c4f06f201d5..981661172251 100644 --- a/src/coinjoin/coinjoin.cpp +++ b/src/coinjoin/coinjoin.cpp @@ -50,7 +50,7 @@ bool CCoinJoinQueue::Sign() uint256 hash = GetSignatureHash(); - CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash); + CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash)); if (!sig.IsValid()) { return false; } @@ -96,7 +96,7 @@ bool CCoinJoinBroadcastTx::Sign() uint256 hash = GetSignatureHash(); - CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash); + CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash)); if (!sig.IsValid()) { return false; } diff --git a/src/evo/mnauth.cpp b/src/evo/mnauth.cpp index deb8a60a79fd..d3f9689c6326 100644 --- a/src/evo/mnauth.cpp +++ b/src/evo/mnauth.cpp @@ -18,31 +18,30 @@ void CMNAuth::PushMNAUTH(CNode* pnode, CConnman& connman) { + LOCK(activeMasternodeInfoCs); if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) { return; } uint256 signHash; - { - LOCK(pnode->cs_mnauth); - if (pnode->receivedMNAuthChallenge.IsNull()) { - return; - } - // We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way - // we protect ourselves against MITM in this form: - // node1 <- Eve -> node2 - // It does not protect against: - // node1 -> Eve -> node2 - // This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff - int nOurNodeVersion{PROTOCOL_VERSION}; - if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) { - nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION); - } - if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) { - signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound)); - } else { - signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion)); - } + auto receivedMNAuthChallenge = pnode->GetReceivedMNAuthChallenge(); + if (receivedMNAuthChallenge.IsNull()) { + return; + } + // We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way + // we protect ourselves against MITM in this form: + // node1 <- Eve -> node2 + // It does not protect against: + // node1 -> Eve -> node2 + // This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff + int nOurNodeVersion{PROTOCOL_VERSION}; + if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) { + nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION); + } + if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) { + signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound)); + } else { + signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion)); } CMNAuth mnauth; @@ -66,11 +65,7 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS vRecv >> mnauth; // only one MNAUTH allowed - bool fAlreadyHaveMNAUTH = false; - { - LOCK(pnode->cs_mnauth); - fAlreadyHaveMNAUTH = !pnode->verifiedProRegTxHash.IsNull(); - } + bool fAlreadyHaveMNAUTH = !pnode->GetVerifiedProRegTxHash().IsNull(); if (fAlreadyHaveMNAUTH) { LOCK(cs_main); Misbehaving(pnode->GetId(), 100, "duplicate mnauth"); @@ -108,20 +103,17 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS } uint256 signHash; - { - LOCK(pnode->cs_mnauth); - int nOurNodeVersion{PROTOCOL_VERSION}; - if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) { - nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION); - } - // See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection) - if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) { - signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound)); - } else { - signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound, pnode->nVersion.load())); - } - LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId()); + int nOurNodeVersion{PROTOCOL_VERSION}; + if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) { + nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION); } + // See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection) + if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) { + signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound)); + } else { + signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound, pnode->nVersion.load())); + } + LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId()); if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash)) { LOCK(cs_main); @@ -147,12 +139,12 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS return; } - if (pnode2->verifiedProRegTxHash == mnauth.proRegTxHash) { + if (pnode2->GetVerifiedProRegTxHash() == mnauth.proRegTxHash) { if (fMasternodeMode) { - auto deterministicOutbound = llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash); + auto deterministicOutbound = WITH_LOCK(activeMasternodeInfoCs, return llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash)); LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode %s has already verified as peer %d, deterministicOutbound=%s. peer=%d\n", mnauth.proRegTxHash.ToString(), pnode2->GetId(), deterministicOutbound.ToString(), pnode->GetId()); - if (deterministicOutbound == activeMasternodeInfo.proTxHash) { + if (WITH_LOCK(activeMasternodeInfoCs, return deterministicOutbound == activeMasternodeInfo.proTxHash)) { if (pnode2->fInbound) { LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping old inbound, peer=%d\n", pnode2->GetId()); pnode2->fDisconnect = true; @@ -181,13 +173,10 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS return; } - { - LOCK(pnode->cs_mnauth); - pnode->verifiedProRegTxHash = mnauth.proRegTxHash; - pnode->verifiedPubKeyHash = dmn->pdmnState->pubKeyOperator.GetHash(); - } + pnode->SetVerifiedProRegTxHash(mnauth.proRegTxHash); + pnode->SetVerifiedPubKeyHash(dmn->pdmnState->pubKeyOperator.GetHash()); - if (!pnode->m_masternode_iqr_connection && connman.IsMasternodeQuorumRelayMember(pnode->verifiedProRegTxHash)) { + if (!pnode->m_masternode_iqr_connection && connman.IsMasternodeQuorumRelayMember(pnode->GetVerifiedProRegTxHash())) { // Tell our peer that we're interested in plain LLMQ recovered signatures. // Otherwise the peer would only announce/send messages resulting from QRECSIG, // e.g. InstantSend locks or ChainLocks. SPV and regular full nodes should not send @@ -209,11 +198,11 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& } g_connman->ForEachNode([&](CNode* pnode) { - LOCK(pnode->cs_mnauth); - if (pnode->verifiedProRegTxHash.IsNull()) { + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); + if (verifiedProRegTxHash.IsNull()) { return; } - auto verifiedDmn = oldMNList.GetMN(pnode->verifiedProRegTxHash); + auto verifiedDmn = oldMNList.GetMN(verifiedProRegTxHash); if (!verifiedDmn) { return; } @@ -223,7 +212,7 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& } else { auto it = diff.updatedMNs.find(verifiedDmn->GetInternalId()); if (it != diff.updatedMNs.end()) { - if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->verifiedPubKeyHash) { + if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->GetVerifiedPubKeyHash()) { doRemove = true; } } @@ -231,7 +220,7 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& if (doRemove) { LogPrint(BCLog::NET_NETCONN, "CMNAuth::NotifyMasternodeListChanged -- Disconnecting MN %s due to key changed/removed, peer=%d\n", - pnode->verifiedProRegTxHash.ToString(), pnode->GetId()); + pnode->GetVerifiedProRegTxHash().ToString(), pnode->GetId()); pnode->fDisconnect = true; } }); diff --git a/src/init.cpp b/src/init.cpp index 3630a3a666b1..0acbd5f11c8a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -363,9 +363,12 @@ void PrepareShutdown() UnregisterValidationInterface(activeMasternodeManager); } - // make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool) - activeMasternodeInfo.blsKeyOperator.reset(); - activeMasternodeInfo.blsPubKeyOperator.reset(); + { + LOCK(activeMasternodeInfoCs); + // make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool) + activeMasternodeInfo.blsKeyOperator.reset(); + activeMasternodeInfo.blsPubKeyOperator.reset(); + } #ifndef WIN32 try { @@ -2323,8 +2326,12 @@ bool AppInitMain() return InitError(_("Invalid masternodeblsprivkey. Please see documentation.")); } fMasternodeMode = true; - activeMasternodeInfo.blsKeyOperator = std::make_unique(keyOperator); - activeMasternodeInfo.blsPubKeyOperator = std::make_unique(activeMasternodeInfo.blsKeyOperator->GetPublicKey()); + { + LOCK(activeMasternodeInfoCs); + activeMasternodeInfo.blsKeyOperator = std::make_unique(keyOperator); + activeMasternodeInfo.blsPubKeyOperator = std::make_unique( + activeMasternodeInfo.blsKeyOperator->GetPublicKey()); + } LogPrintf("MASTERNODE:\n"); LogPrintf(" blsPubKeyOperator: %s\n", keyOperator.GetPublicKey().ToString()); } @@ -2335,11 +2342,14 @@ bool AppInitMain() RegisterValidationInterface(activeMasternodeManager); } - if (activeMasternodeInfo.blsKeyOperator == nullptr) { - activeMasternodeInfo.blsKeyOperator = std::make_unique(); - } - if (activeMasternodeInfo.blsPubKeyOperator == nullptr) { - activeMasternodeInfo.blsPubKeyOperator = std::make_unique(); + { + LOCK(activeMasternodeInfoCs); + if (activeMasternodeInfo.blsKeyOperator == nullptr) { + activeMasternodeInfo.blsKeyOperator = std::make_unique(); + } + if (activeMasternodeInfo.blsPubKeyOperator == nullptr) { + activeMasternodeInfo.blsPubKeyOperator = std::make_unique(); + } } // ********************************************************* Step 10b: setup CoinJoin diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index dd3a3fac7259..2555efed7d73 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -61,7 +61,10 @@ void CQuorum::Init(const CFinalCommitmentPtr& _qc, const CBlockIndex* _pindexQuo bool CQuorum::SetVerificationVector(const BLSVerificationVector& quorumVecIn) { - if (::SerializeHash(quorumVecIn) != qc->quorumVvecHash) { + const auto quorumVecInSerialized = ::SerializeHash(quorumVecIn); + + LOCK(cs); + if (quorumVecInSerialized != qc->quorumVvecHash) { return false; } quorumVvec = std::make_shared(quorumVecIn); @@ -70,9 +73,10 @@ bool CQuorum::SetVerificationVector(const BLSVerificationVector& quorumVecIn) bool CQuorum::SetSecretKeyShare(const CBLSSecretKey& secretKeyShare) { - if (!secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(GetMemberIndex(activeMasternodeInfo.proTxHash)))) { + if (!secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(WITH_LOCK(activeMasternodeInfoCs, return GetMemberIndex(activeMasternodeInfo.proTxHash))))) { return false; } + LOCK(cs); skShare = secretKeyShare; return true; } @@ -99,15 +103,22 @@ bool CQuorum::IsValidMember(const uint256& proTxHash) const CBLSPublicKey CQuorum::GetPubKeyShare(size_t memberIdx) const { - if (quorumVvec == nullptr || memberIdx >= members.size() || !qc->validMembers[memberIdx]) { + LOCK(cs); + if (!HasVerificationVector() || memberIdx >= members.size() || !qc->validMembers[memberIdx]) { return CBLSPublicKey(); } auto& m = members[memberIdx]; return blsCache.BuildPubKeyShare(m->proTxHash, quorumVvec, CBLSId(m->proTxHash)); } -const CBLSSecretKey& CQuorum::GetSkShare() const +bool CQuorum::HasVerificationVector() const { + LOCK(cs); + return quorumVvec != nullptr; +} + +CBLSSecretKey CQuorum::GetSkShare() const { + LOCK(cs); return skShare; } @@ -125,7 +136,8 @@ void CQuorum::WriteContributions(CEvoDB& evoDb) const { uint256 dbKey = MakeQuorumKey(*this); - if (quorumVvec != nullptr) { + LOCK(cs); + if (HasVerificationVector()) { evoDb.GetRawDB().Write(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), *quorumVvec); } if (skShare.IsValid()) { @@ -139,14 +151,14 @@ bool CQuorum::ReadContributions(CEvoDB& evoDb) BLSVerificationVector qv; if (evoDb.Read(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), qv)) { - quorumVvec = std::make_shared(std::move(qv)); + WITH_LOCK(cs, quorumVvec = std::make_shared(std::move(qv))); } else { return false; } // We ignore the return value here as it is ok if this fails. If it fails, it usually means that we are not a // member of the quorum but observed the whole DKG process to have the quorum verification vector. - evoDb.Read(std::make_pair(DB_QUORUM_SK_SHARE, dbKey), skShare); + WITH_LOCK(cs, evoDb.Read(std::make_pair(DB_QUORUM_SK_SHARE, dbKey), skShare)); return true; } @@ -197,8 +209,10 @@ void CQuorumManager::TriggerQuorumDataRecoveryThreads(const CBlockIndex* pIndex) // First check if we are member of any quorum of this type bool fWeAreQuorumTypeMember{false}; + + auto proTxHash = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash); for (const auto& pQuorum : vecQuorums) { - if (pQuorum->IsValidMember(activeMasternodeInfo.proTxHash)) { + if (pQuorum->IsValidMember(proTxHash)) { fWeAreQuorumTypeMember = true; break; } @@ -211,16 +225,16 @@ void CQuorumManager::TriggerQuorumDataRecoveryThreads(const CBlockIndex* pIndex) } uint16_t nDataMask{0}; - const bool fWeAreQuorumMember = pQuorum->IsValidMember(activeMasternodeInfo.proTxHash); + const bool fWeAreQuorumMember = pQuorum->IsValidMember(proTxHash); const bool fSyncForTypeEnabled = mapQuorumVvecSync.count(pQuorum->qc->llmqType) > 0; const QvvecSyncMode syncMode = fSyncForTypeEnabled ? mapQuorumVvecSync.at(pQuorum->qc->llmqType) : QvvecSyncMode::Invalid; const bool fSyncCurrent = syncMode == QvvecSyncMode::Always || (syncMode == QvvecSyncMode::OnlyIfTypeMember && fWeAreQuorumTypeMember); - if ((fWeAreQuorumMember || (fSyncForTypeEnabled && fSyncCurrent)) && pQuorum->quorumVvec == nullptr) { + if ((fWeAreQuorumMember || (fSyncForTypeEnabled && fSyncCurrent)) && !pQuorum->HasVerificationVector()) { nDataMask |= llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR; } - if (fWeAreQuorumMember && !pQuorum->skShare.IsValid()) { + if (fWeAreQuorumMember && !pQuorum->GetSkShare().IsValid()) { nDataMask |= llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS; } @@ -266,7 +280,6 @@ void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const { const auto& llmq_params = GetLLMQParams(llmqType); - const auto& myProTxHash = activeMasternodeInfo.proTxHash; auto lastQuorums = ScanQuorums(llmqType, pindexNew, (size_t)llmq_params.keepOldConnections); auto connmanQuorumsToDelete = g_connman->GetMasternodeQuorums(llmqType); @@ -277,7 +290,7 @@ void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const connmanQuorumsToDelete.erase(curDkgBlock); for (const auto& quorum : lastQuorums) { - if (CLLMQUtils::EnsureQuorumConnections(llmqType, quorum->pindexQuorum, myProTxHash)) { + if (CLLMQUtils::EnsureQuorumConnections(llmqType, quorum->pindexQuorum, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash))) { continue; } if (connmanQuorumsToDelete.count(quorum->qc->quorumHash) > 0) { @@ -339,8 +352,9 @@ bool CQuorumManager::BuildQuorumContributions(const CFinalCommitmentPtr& fqc, co } cxxtimer::Timer t2(true); + LOCK(quorum->cs); quorum->quorumVvec = blsWorker.BuildQuorumVerificationVector(vvecs); - if (quorum->quorumVvec == nullptr) { + if (!quorum->HasVerificationVector()) { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- failed to build quorumVvec\n", __func__); // without the quorum vvec, there can't be a skShare, so we fail here. Failure is not fatal here, as it still // allows to use the quorum as a non-member (verification through the quorum pub key) @@ -371,7 +385,7 @@ bool CQuorumManager::RequestQuorumData(CNode* pFrom, Consensus::LLMQType llmqTyp LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- Version must be %d or greater.\n", __func__, LLMQ_DATA_MESSAGES_VERSION); return false; } - if (pFrom == nullptr || (pFrom->verifiedProRegTxHash.IsNull() && !pFrom->qwatch)) { + if (pFrom == nullptr || (pFrom->GetVerifiedProRegTxHash().IsNull() && !pFrom->qwatch)) { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- pFrom is neither a verified masternode nor a qwatch connection\n", __func__); return false; } @@ -389,7 +403,7 @@ bool CQuorumManager::RequestQuorumData(CNode* pFrom, Consensus::LLMQType llmqTyp } LOCK(cs_data_requests); - auto key = std::make_pair(pFrom->verifiedProRegTxHash, true); + auto key = std::make_pair(pFrom->GetVerifiedProRegTxHash(), true); auto it = mapQuorumDataRequests.emplace(key, CQuorumDataRequest(llmqType, pQuorumIndex->GetBlockHash(), nDataMask, proTxHash)); if (!it.second && !it.first->second.IsExpired()) { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- Already requested\n", __func__); @@ -517,10 +531,13 @@ size_t CQuorumManager::GetQuorumRecoveryStartOffset(const CQuorumCPtr pQuorum, c }); std::sort(vecProTxHashes.begin(), vecProTxHashes.end()); size_t nIndex{0}; - for (size_t i = 0; i < vecProTxHashes.size(); ++i) { - if (activeMasternodeInfo.proTxHash == vecProTxHashes[i]) { - nIndex = i; - break; + { + LOCK(activeMasternodeInfoCs); + for (size_t i = 0; i < vecProTxHashes.size(); ++i) { + if (activeMasternodeInfo.proTxHash == vecProTxHashes[i]) { + nIndex = i; + break; + } } } return nIndex % pQuorum->qc->validMembers.size(); @@ -539,7 +556,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, if (strCommand == NetMsgType::QGETDATA) { - if (!fMasternodeMode || pFrom == nullptr || (pFrom->verifiedProRegTxHash.IsNull() && !pFrom->qwatch)) { + if (!fMasternodeMode || pFrom == nullptr || (pFrom->GetVerifiedProRegTxHash().IsNull() && !pFrom->qwatch)) { errorHandler("Not a verified masternode or a qwatch connection"); return; } @@ -556,7 +573,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, { LOCK2(cs_main, cs_data_requests); - auto key = std::make_pair(pFrom->verifiedProRegTxHash, false); + auto key = std::make_pair(pFrom->GetVerifiedProRegTxHash(), false); auto it = mapQuorumDataRequests.find(key); if (it == mapQuorumDataRequests.end()) { it = mapQuorumDataRequests.emplace(key, request).first; @@ -592,13 +609,12 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, // Check if request wants QUORUM_VERIFICATION_VECTOR data if (request.GetDataMask() & CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR) { - - if (!pQuorum->quorumVvec) { + if (!pQuorum->HasVerificationVector()) { sendQDATA(CQuorumDataRequest::Errors::QUORUM_VERIFICATION_VECTOR_MISSING); return; } - ssResponseData << *pQuorum->quorumVvec; + WITH_LOCK(pQuorum->cs, ssResponseData << *pQuorum->quorumVvec); } // Check if request wants ENCRYPTED_CONTRIBUTIONS data @@ -624,8 +640,8 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, } if (strCommand == NetMsgType::QDATA) { - - if ((!fMasternodeMode && !CLLMQUtils::IsWatchQuorumsEnabled()) || pFrom == nullptr || (pFrom->verifiedProRegTxHash.IsNull() && !pFrom->qwatch)) { + auto verifiedProRegTxHash = pFrom->GetVerifiedProRegTxHash(); + if ((!fMasternodeMode && !CLLMQUtils::IsWatchQuorumsEnabled()) || pFrom == nullptr || (verifiedProRegTxHash.IsNull() && !pFrom->qwatch)) { errorHandler("Not a verified masternode or a qwatch connection"); return; } @@ -635,7 +651,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, { LOCK2(cs_main, cs_data_requests); - auto it = mapQuorumDataRequests.find(std::make_pair(pFrom->verifiedProRegTxHash, true)); + auto it = mapQuorumDataRequests.find(std::make_pair(verifiedProRegTxHash, true)); if (it == mapQuorumDataRequests.end()) { errorHandler("Not requested"); return; @@ -682,7 +698,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, // Check if request has ENCRYPTED_CONTRIBUTIONS data if (request.GetDataMask() & CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS) { - if (pQuorum->quorumVvec->size() != pQuorum->params.threshold) { + if (WITH_LOCK(pQuorum->cs, return pQuorum->quorumVvec->size() != pQuorum->params.threshold)) { errorHandler("No valid quorum verification vector available", 0); // Don't bump score because we asked for it return; } @@ -698,8 +714,9 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, BLSSecretKeyVector vecSecretKeys; vecSecretKeys.resize(vecEncrypted.size()); + auto secret = WITH_LOCK(activeMasternodeInfoCs, return *activeMasternodeInfo.blsKeyOperator); for (size_t i = 0; i < vecEncrypted.size(); ++i) { - if (!vecEncrypted[i].Decrypt(memberIdx, *activeMasternodeInfo.blsKeyOperator, vecSecretKeys[i], PROTOCOL_VERSION)) { + if (!vecEncrypted[i].Decrypt(memberIdx, secret, vecSecretKeys[i], PROTOCOL_VERSION)) { errorHandler("Failed to decrypt"); return; } @@ -718,7 +735,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand, void CQuorumManager::StartCachePopulatorThread(const CQuorumCPtr pQuorum) const { - if (pQuorum->quorumVvec == nullptr) { + if (!pQuorum->HasVerificationVector()) { return; } @@ -771,7 +788,7 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co vecMemberHashes.reserve(pQuorum->qc->validMembers.size()); for (auto& member : pQuorum->members) { - if (pQuorum->IsValidMember(member->proTxHash) && member->proTxHash != activeMasternodeInfo.proTxHash) { + if (pQuorum->IsValidMember(member->proTxHash) && member->proTxHash != WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash)) { vecMemberHashes.push_back(member->proTxHash); } } @@ -781,12 +798,13 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co while (nDataMask > 0 && !quorumThreadInterrupt) { - if (nDataMask & llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR && pQuorum->quorumVvec != nullptr) { + if (nDataMask & llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR && + pQuorum->HasVerificationVector()) { nDataMask &= ~llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR; printLog("Received quorumVvec"); } - if (nDataMask & llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS && pQuorum->skShare.IsValid()) { + if (nDataMask & llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS && pQuorum->GetSkShare().IsValid()) { nDataMask &= ~llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS; printLog("Received skShare"); } @@ -818,18 +836,19 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co printLog("Connect"); } + auto proTxHash = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash); g_connman->ForEachNode([&](CNode* pNode) { - - if (pCurrentMemberHash == nullptr || pNode->verifiedProRegTxHash != *pCurrentMemberHash) { + auto verifiedProRegTxHash = pNode->GetVerifiedProRegTxHash(); + if (pCurrentMemberHash == nullptr || verifiedProRegTxHash != *pCurrentMemberHash) { return; } - if (quorumManager->RequestQuorumData(pNode, pQuorum->qc->llmqType, pQuorum->pindexQuorum, nDataMask, activeMasternodeInfo.proTxHash)) { + if (quorumManager->RequestQuorumData(pNode, pQuorum->qc->llmqType, pQuorum->pindexQuorum, nDataMask, proTxHash)) { nTimeLastSuccess = GetAdjustedTime(); printLog("Requested"); } else { LOCK(cs_data_requests); - auto it = mapQuorumDataRequests.find(std::make_pair(pNode->verifiedProRegTxHash, true)); + auto it = mapQuorumDataRequests.find(std::make_pair(verifiedProRegTxHash, true)); if (it == mapQuorumDataRequests.end()) { printLog("Failed"); pNode->fDisconnect = true; diff --git a/src/llmq/quorums.h b/src/llmq/quorums.h index f9f1c007da92..6faebf079ed5 100644 --- a/src/llmq/quorums.h +++ b/src/llmq/quorums.h @@ -157,16 +157,17 @@ class CQuorum uint256 minedBlockHash; std::vector members; - // These are only valid when we either participated in the DKG or fully watched it - BLSVerificationVectorPtr quorumVvec; - CBLSSecretKey skShare; - private: // Recovery of public key shares is very slow, so we start a background thread that pre-populates a cache so that // the public key shares are ready when needed later mutable CBLSWorkerCache blsCache; mutable std::atomic fQuorumDataRecoveryThreadRunning{false}; + mutable CCriticalSection cs; + // These are only valid when we either participated in the DKG or fully watched it + BLSVerificationVectorPtr quorumVvec GUARDED_BY(cs); + CBLSSecretKey skShare GUARDED_BY(cs); + public: CQuorum(const Consensus::LLMQParams& _params, CBLSWorker& _blsWorker); ~CQuorum(); @@ -175,12 +176,13 @@ class CQuorum bool SetVerificationVector(const BLSVerificationVector& quorumVecIn); bool SetSecretKeyShare(const CBLSSecretKey& secretKeyShare); + bool HasVerificationVector() const; bool IsMember(const uint256& proTxHash) const; bool IsValidMember(const uint256& proTxHash) const; int GetMemberIndex(const uint256& proTxHash) const; CBLSPublicKey GetPubKeyShare(size_t memberIdx) const; - const CBLSSecretKey& GetSkShare() const; + CBLSSecretKey GetSkShare() const; private: void WriteContributions(CEvoDB& evoDb) const; diff --git a/src/llmq/quorums_dkgsession.cpp b/src/llmq/quorums_dkgsession.cpp index f001ace91391..5bac24e423c5 100644 --- a/src/llmq/quorums_dkgsession.cpp +++ b/src/llmq/quorums_dkgsession.cpp @@ -199,7 +199,7 @@ void CDKGSession::SendContributions(CDKGPendingMessages& pendingMessages) logger.Batch("encrypted contributions. time=%d", t1.count()); - qc.sig = activeMasternodeInfo.blsKeyOperator->Sign(qc.GetSignHash()); + qc.sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(qc.GetSignHash())); logger.Flush(); @@ -324,7 +324,7 @@ void CDKGSession::ReceiveMessage(const CDKGContribution& qc, bool& retBan) bool complain = false; CBLSSecretKey skContribution; - if (!qc.contributions->Decrypt(myIdx, *activeMasternodeInfo.blsKeyOperator, skContribution, PROTOCOL_VERSION)) { + if (!qc.contributions->Decrypt(myIdx, WITH_LOCK(activeMasternodeInfoCs, return *activeMasternodeInfo.blsKeyOperator), skContribution, PROTOCOL_VERSION)) { logger.Batch("contribution from %s could not be decrypted", member->dmn->proTxHash.ToString()); complain = true; } else if (member->idx != myIdx && ShouldSimulateError("complain-lie")) { @@ -466,10 +466,11 @@ void CDKGSession::VerifyConnectionAndMinProtoVersions() const std::unordered_map protoMap; g_connman->ForEachNode([&](const CNode* pnode) { - if (pnode->verifiedProRegTxHash.IsNull()) { + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); + if (verifiedProRegTxHash.IsNull()) { return; } - protoMap.emplace(pnode->verifiedProRegTxHash, pnode->nVersion); + protoMap.emplace(verifiedProRegTxHash, pnode->nVersion); }); bool fShouldAllMembersBeConnected = CLLMQUtils::IsAllMembersConnectedEnabled(params.type); @@ -525,7 +526,7 @@ void CDKGSession::SendComplaint(CDKGPendingMessages& pendingMessages) logger.Batch("sending complaint. badCount=%d, complaintCount=%d", badCount, complaintCount); - qc.sig = activeMasternodeInfo.blsKeyOperator->Sign(qc.GetSignHash()); + qc.sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(qc.GetSignHash())); logger.Flush(); @@ -725,7 +726,7 @@ void CDKGSession::SendJustification(CDKGPendingMessages& pendingMessages, const return; } - qj.sig = activeMasternodeInfo.blsKeyOperator->Sign(qj.GetSignHash()); + qj.sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(qj.GetSignHash())); logger.Flush(); @@ -1027,7 +1028,7 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages) (*commitmentHash.begin())++; } - qc.sig = activeMasternodeInfo.blsKeyOperator->Sign(commitmentHash); + qc.sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(commitmentHash)); qc.quorumSig = skShare.Sign(commitmentHash); if (lieType == 3) { @@ -1334,9 +1335,10 @@ void CDKGSession::RelayInvToParticipants(const CInv& inv) const LOCK(invCs); g_connman->ForEachNode([&](CNode* pnode) { bool relay = false; + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); if (pnode->qwatch) { relay = true; - } else if (!pnode->verifiedProRegTxHash.IsNull() && relayMembers.count(pnode->verifiedProRegTxHash)) { + } else if (!verifiedProRegTxHash.IsNull() && relayMembers.count(verifiedProRegTxHash)) { relay = true; } if (relay) { diff --git a/src/llmq/quorums_dkgsessionhandler.cpp b/src/llmq/quorums_dkgsessionhandler.cpp index f8097b4405a0..2b409fabac3e 100644 --- a/src/llmq/quorums_dkgsessionhandler.cpp +++ b/src/llmq/quorums_dkgsessionhandler.cpp @@ -166,7 +166,7 @@ bool CDKGSessionHandler::InitNewQuorum(const CBlockIndex* pindexQuorum) auto mns = CLLMQUtils::GetAllQuorumMembers(params.type, pindexQuorum); - if (!curSession->Init(pindexQuorum, mns, activeMasternodeInfo.proTxHash)) { + if (!curSession->Init(pindexQuorum, mns, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash))) { LogPrintf("CDKGSessionManager::%s -- quorum initialization failed for %s\n", __func__, curSession->params.name); return false; } diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 70c82783fce1..2c029421475a 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -784,7 +784,7 @@ void CSigningManager::UnregisterRecoveredSigsListener(CRecoveredSigsListener* l) bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash, const uint256& quorumHash, bool allowReSign) { - if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) { + if (!fMasternodeMode || WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash.IsNull())) { return false; } @@ -805,7 +805,7 @@ bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, const uint return false; } - if (!quorum->IsValidMember(activeMasternodeInfo.proTxHash)) { + if (!WITH_LOCK(activeMasternodeInfoCs, return quorum->IsValidMember(activeMasternodeInfo.proTxHash))) { return false; } diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp index c79ded4a3f2c..fb241b00598c 100644 --- a/src/llmq/quorums_signing_shares.cpp +++ b/src/llmq/quorums_signing_shares.cpp @@ -234,7 +234,7 @@ void CSigSharesManager::InterruptWorkerThread() void CSigSharesManager::ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv) { // non-masternodes are not interested in sigshares - if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) { + if (!fMasternodeMode || WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash.IsNull())) { return; } @@ -372,7 +372,7 @@ bool CSigSharesManager::ProcessMessageSigSharesInv(CNode* pfrom, const CSigShare LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- signHash=%s, inv={%s}, node=%d\n", __func__, sessionInfo.signHash.ToString(), inv.ToString(), pfrom->GetId()); - if (sessionInfo.quorum->quorumVvec == nullptr) { + if (!sessionInfo.quorum->HasVerificationVector()) { // TODO we should allow to ask other nodes for the quorum vvec if we missed it in the DKG LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- we don't have the quorum vvec for %s, not requesting sig shares. node=%d\n", __func__, sessionInfo.quorumHash.ToString(), pfrom->GetId()); @@ -485,11 +485,11 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s // quorum is too old return; } - if (!quorum->IsMember(activeMasternodeInfo.proTxHash)) { + if (!quorum->IsMember(WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash))) { // we're not a member so we can't verify it (we actually shouldn't have received it) return; } - if (quorum->quorumVvec == nullptr) { + if (!quorum->HasVerificationVector()) { // TODO we should allow to ask other nodes for the quorum vvec if we missed it in the DKG LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- we don't have the quorum vvec for %s, no verification possible. node=%d\n", __func__, quorum->qc->quorumHash.ToString(), fromId); @@ -534,11 +534,11 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(const CSigSharesNodeState::Ses // quorum is too old return false; } - if (!session.quorum->IsMember(activeMasternodeInfo.proTxHash)) { + if (!session.quorum->IsMember(WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash))) { // we're not a member so we can't verify it (we actually shouldn't have received it) return false; } - if (session.quorum->quorumVvec == nullptr) { + if (!session.quorum->HasVerificationVector()) { // TODO we should allow to ask other nodes for the quorum vvec if we missed it in the DKG LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- we don't have the quorum vvec for %s, no verification possible.\n", __func__, session.quorumHash.ToString()); @@ -730,7 +730,7 @@ void CSigSharesManager::ProcessSigShare(const CSigShare& sigShare, CConnman& con // prepare node set for direct-push in case this is our sig share std::set quorumNodes; - if (!CLLMQUtils::IsAllMembersConnectedEnabled(llmqType) && sigShare.quorumMember == quorum->GetMemberIndex(activeMasternodeInfo.proTxHash)) { + if (!CLLMQUtils::IsAllMembersConnectedEnabled(llmqType) && sigShare.quorumMember == quorum->GetMemberIndex(WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash))) { quorumNodes = connman.GetMasternodeQuorumNodes(sigShare.llmqType, sigShare.quorumHash); } @@ -1018,10 +1018,11 @@ void CSigSharesManager::CollectSigSharesToSendConcentrated(std::unordered_map proTxToNode; for (const auto& pnode : vNodes) { - if (pnode->verifiedProRegTxHash.IsNull()) { + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); + if (verifiedProRegTxHash.IsNull()) { continue; } - proTxToNode.emplace(pnode->verifiedProRegTxHash, pnode); + proTxToNode.emplace(verifiedProRegTxHash, pnode); } auto curTime = GetTime().count(); @@ -1567,8 +1568,9 @@ void CSigSharesManager::SignPendingSigShares() CSigShare CSigSharesManager::CreateSigShare(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash) const { cxxtimer::Timer t(true); + auto activeMasterNodeProTxHash = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash); - if (!quorum->IsValidMember(activeMasternodeInfo.proTxHash)) { + if (!quorum->IsValidMember(activeMasterNodeProTxHash)) { return {}; } @@ -1578,7 +1580,7 @@ CSigShare CSigSharesManager::CreateSigShare(const CQuorumCPtr& quorum, const uin return {}; } - int memberIdx = quorum->GetMemberIndex(activeMasternodeInfo.proTxHash); + int memberIdx = quorum->GetMemberIndex(activeMasterNodeProTxHash); if (memberIdx == -1) { // this should really not happen (IsValidMember gave true) return {}; diff --git a/src/masternode/activemasternode.cpp b/src/masternode/activemasternode.cpp index 61f0b828d531..43d477a29e6f 100644 --- a/src/masternode/activemasternode.cpp +++ b/src/masternode/activemasternode.cpp @@ -15,7 +15,8 @@ #include // Keep track of the active Masternode -CActiveMasternodeInfo activeMasternodeInfo; +CCriticalSection activeMasternodeInfoCs; +CActiveMasternodeInfo activeMasternodeInfo GUARDED_BY(activeMasternodeInfoCs); CActiveMasternodeManager* activeMasternodeManager; std::string CActiveMasternodeManager::GetStateString() const @@ -64,7 +65,7 @@ std::string CActiveMasternodeManager::GetStatus() const void CActiveMasternodeManager::Init(const CBlockIndex* pindex) { - LOCK(cs_main); + LOCK2(cs_main, activeMasternodeInfoCs); if (!fMasternodeMode) return; @@ -136,7 +137,7 @@ void CActiveMasternodeManager::Init(const CBlockIndex* pindex) void CActiveMasternodeManager::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) { - LOCK(cs_main); + LOCK2(cs_main, activeMasternodeInfoCs); if (!fMasternodeMode) return; @@ -201,10 +202,11 @@ bool CActiveMasternodeManager::GetLocalAddress(CService& addrRet) if (!fFoundLocal) { bool empty = true; // If we have some peers, let's try to find our local address from one of them - g_connman->ForEachNodeContinueIf(CConnman::AllNodes, [&fFoundLocal, &empty](CNode* pnode) { + auto service = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.service); + g_connman->ForEachNodeContinueIf(CConnman::AllNodes, [&](CNode* pnode) { empty = false; if (pnode->addr.IsIPv4()) - fFoundLocal = GetLocal(activeMasternodeInfo.service, &pnode->addr) && IsValidNetAddr(activeMasternodeInfo.service); + fFoundLocal = GetLocal(service, &pnode->addr) && IsValidNetAddr(service); return !fFoundLocal; }); // nothing and no live connections, can't do anything for now diff --git a/src/masternode/activemasternode.h b/src/masternode/activemasternode.h index 5eee35955b28..5991e6db8fbc 100644 --- a/src/masternode/activemasternode.h +++ b/src/masternode/activemasternode.h @@ -16,6 +16,7 @@ struct CActiveMasternodeInfo; class CActiveMasternodeManager; extern CActiveMasternodeInfo activeMasternodeInfo; +extern CCriticalSection activeMasternodeInfoCs; extern CActiveMasternodeManager* activeMasternodeManager; struct CActiveMasternodeInfo { diff --git a/src/net.cpp b/src/net.cpp index 6f66f9d36243..7a3ca1f5e185 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1108,7 +1108,7 @@ bool CConnman::AttemptToEvictConnection() } // if MNAUTH was valid, the node is always protected (and at the same time not accounted when // checking incoming connection limits) - if (!node->verifiedProRegTxHash.IsNull()) { + if (!node->GetVerifiedProRegTxHash().IsNull()) { isProtected = true; } if (isProtected) { @@ -1198,7 +1198,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { for (const CNode* pnode : vNodes) { if (pnode->fInbound) { nInbound++; - if (!pnode->verifiedProRegTxHash.IsNull()) { + if (!pnode->GetVerifiedProRegTxHash().IsNull()) { nVerifiedInboundMasternodes++; } } @@ -2408,8 +2408,9 @@ void CConnman::ThreadOpenConnections(const std::vector connect) { LOCK(cs_vNodes); for (CNode* pnode : vNodes) { - if (!pnode->verifiedProRegTxHash.IsNull()) { - setConnectedMasternodes.emplace(pnode->verifiedProRegTxHash); + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); + if (!verifiedProRegTxHash.IsNull()) { + setConnectedMasternodes.emplace(verifiedProRegTxHash); } } } @@ -2632,9 +2633,10 @@ void CConnman::ThreadOpenMasternodeConnections() std::set connectedNodes; std::map connectedProRegTxHashes; ForEachNode([&](const CNode* pnode) { + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); connectedNodes.emplace(pnode->addr); - if (!pnode->verifiedProRegTxHash.IsNull()) { - connectedProRegTxHashes.emplace(pnode->verifiedProRegTxHash, pnode->fInbound); + if (!verifiedProRegTxHash.IsNull()) { + connectedProRegTxHashes.emplace(verifiedProRegTxHash, pnode->fInbound); } }); @@ -3519,7 +3521,8 @@ void CConnman::SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, con // Update existing connections ForEachNode([&](CNode* pnode) { - if (!pnode->verifiedProRegTxHash.IsNull() && !pnode->m_masternode_iqr_connection && IsMasternodeQuorumRelayMember(pnode->verifiedProRegTxHash)) { + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); + if (!verifiedProRegTxHash.IsNull() && !pnode->m_masternode_iqr_connection && IsMasternodeQuorumRelayMember(verifiedProRegTxHash)) { // Tell our peer that we're interested in plain LLMQ recovered signatures. // Otherwise the peer would only announce/send messages resulting from QRECSIG, // e.g. InstantSend locks or ChainLocks. SPV and regular full nodes should not send @@ -3564,7 +3567,8 @@ std::set CConnman::GetMasternodeQuorumNodes(Consensus::LLMQType llmqType if (pnode->fDisconnect) { continue; } - if (!pnode->qwatch && (pnode->verifiedProRegTxHash.IsNull() || !proRegTxHashes.count(pnode->verifiedProRegTxHash))) { + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); + if (!pnode->qwatch && (verifiedProRegTxHash.IsNull() || !proRegTxHashes.count(verifiedProRegTxHash))) { continue; } nodes.emplace(pnode->GetId()); @@ -3584,7 +3588,7 @@ bool CConnman::IsMasternodeQuorumNode(const CNode* pnode) // Let's see if this is an outgoing connection to an address that is known to be a masternode // We however only need to know this if the node did not authenticate itself as a MN yet uint256 assumedProTxHash; - if (pnode->verifiedProRegTxHash.IsNull() && !pnode->fInbound) { + if (pnode->GetVerifiedProRegTxHash().IsNull() && !pnode->fInbound) { auto mnList = deterministicMNManager->GetListAtChainTip(); auto dmn = mnList.GetMNByService(pnode->addr); if (dmn == nullptr) { @@ -3596,8 +3600,8 @@ bool CConnman::IsMasternodeQuorumNode(const CNode* pnode) LOCK(cs_vPendingMasternodes); for (const auto& p : masternodeQuorumNodes) { - if (!pnode->verifiedProRegTxHash.IsNull()) { - if (p.second.count(pnode->verifiedProRegTxHash)) { + if (!pnode->GetVerifiedProRegTxHash().IsNull()) { + if (p.second.count(pnode->GetVerifiedProRegTxHash())) { return true; } } else if (!assumedProTxHash.IsNull()) { diff --git a/src/net.h b/src/net.h index b86e87086695..9b1e6841f073 100644 --- a/src/net.h +++ b/src/net.h @@ -904,11 +904,11 @@ class CNode bool fRelayTxes; //protected by cs_filter bool fSentAddr; // If 'true' this node will be disconnected on CMasternodeMan::ProcessMasternodeConnections() - bool m_masternode_connection; + std::atomic m_masternode_connection; // If 'true' this node will be disconnected after MNAUTH - bool m_masternode_probe_connection; + std::atomic m_masternode_probe_connection; // If 'true', we identified it as an intra-quorum relay connection - bool m_masternode_iqr_connection{false}; + std::atomic m_masternode_iqr_connection{false}; CSemaphoreGrant grantOutbound; CCriticalSection cs_filter; std::unique_ptr pfilter PT_GUARDED_BY(cs_filter){nullptr}; @@ -979,13 +979,6 @@ class CNode // If true, we will send him CoinJoin queue messages std::atomic fSendDSQueue{false}; - // Challenge sent in VERSION to be answered with MNAUTH (only happens between MNs) - mutable CCriticalSection cs_mnauth; - uint256 sentMNAuthChallenge; - uint256 receivedMNAuthChallenge; - uint256 verifiedProRegTxHash; - uint256 verifiedPubKeyHash; - // If true, we will announce/send him plain recovered sigs (usually true for full nodes) std::atomic fSendRecSigs{false}; // If true, we will send him all quorum related messages, even if he is not a member of our quorums @@ -1013,6 +1006,14 @@ class CNode // Our address, as reported by the peer CService addrLocal GUARDED_BY(cs_addrLocal); mutable CCriticalSection cs_addrLocal; + + // Challenge sent in VERSION to be answered with MNAUTH (only happens between MNs) + mutable CCriticalSection cs_mnauth; + uint256 sentMNAuthChallenge GUARDED_BY(cs_mnauth); + uint256 receivedMNAuthChallenge GUARDED_BY(cs_mnauth); + uint256 verifiedProRegTxHash GUARDED_BY(cs_mnauth); + uint256 verifiedPubKeyHash GUARDED_BY(cs_mnauth); + public: NodeId GetId() const { @@ -1146,6 +1147,46 @@ class CNode std::string GetLogString() const; bool CanRelay() const { return !m_masternode_connection || m_masternode_iqr_connection; } + + uint256 GetSentMNAuthChallenge() const { + LOCK(cs_mnauth); + return sentMNAuthChallenge; + } + + uint256 GetReceivedMNAuthChallenge() const { + LOCK(cs_mnauth); + return receivedMNAuthChallenge; + } + + uint256 GetVerifiedProRegTxHash() const { + LOCK(cs_mnauth); + return verifiedProRegTxHash; + } + + uint256 GetVerifiedPubKeyHash() const { + LOCK(cs_mnauth); + return verifiedPubKeyHash; + } + + void SetSentMNAuthChallenge(const uint256& newSentMNAuthChallenge) { + LOCK(cs_mnauth); + sentMNAuthChallenge = newSentMNAuthChallenge; + } + + void SetReceivedMNAuthChallenge(const uint256& newReceivedMNAuthChallenge) { + LOCK(cs_mnauth); + receivedMNAuthChallenge = newReceivedMNAuthChallenge; + } + + void SetVerifiedProRegTxHash(const uint256& newVerifiedProRegTxHash) { + LOCK(cs_mnauth); + verifiedProRegTxHash = newVerifiedProRegTxHash; + } + + void SetVerifiedPubKeyHash(const uint256& newVerifiedPubKeyHash) { + LOCK(cs_mnauth); + verifiedPubKeyHash = newVerifiedPubKeyHash; + } }; class CExplicitNetCleanup diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f5010e39caf5..7f37fa1b9191 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -434,10 +434,7 @@ static void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime) uint256 mnauthChallenge; GetRandBytes(mnauthChallenge.begin(), mnauthChallenge.size()); - { - LOCK(pnode->cs_mnauth); - pnode->sentMNAuthChallenge = mnauthChallenge; - } + pnode->SetSentMNAuthChallenge(mnauthChallenge); int nProtocolVersion = PROTOCOL_VERSION; if (params.NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) { @@ -445,7 +442,7 @@ static void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime) } connman->PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, nProtocolVersion, (uint64_t)nLocalNodeServices, nTime, addrYou, addrMe, - nonce, strSubVersion, nNodeStartingHeight, ::fRelayTxes, mnauthChallenge, pnode->m_masternode_connection)); + nonce, strSubVersion, nNodeStartingHeight, ::fRelayTxes, mnauthChallenge, pnode->m_masternode_connection.load())); if (fLogIPs) { LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, them=%s, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), addrYou.ToString(), nodeid); @@ -2208,8 +2205,9 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (!vRecv.empty()) vRecv >> fRelay; if (!vRecv.empty()) { - LOCK(pfrom->cs_mnauth); - vRecv >> pfrom->receivedMNAuthChallenge; + uint256 receivedMNAuthChallenge; + vRecv >> receivedMNAuthChallenge; + pfrom->SetReceivedMNAuthChallenge(receivedMNAuthChallenge); } if (!vRecv.empty()) { bool fOtherMasternode = false; @@ -4235,7 +4233,6 @@ bool PeerLogicValidation::SendMessages(CNode* pto) std::vector vInv; { LOCK2(mempool.cs, pto->cs_inventory); - size_t reserve = std::min(pto->setInventoryTxToSend.size(), INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000); reserve = std::max(reserve, pto->vInventoryBlockToSend.size()); reserve = std::min(reserve, MAX_INV_SZ); @@ -4262,7 +4259,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) } else { // Use half the delay for regular outbound peers, as there is less privacy concern for them. // and quarter the delay for Masternode outbound peers, as there is even less privacy concern in this case. - pto->nNextInvSend = PoissonNextSend(current_time, std::chrono::seconds{INVENTORY_BROADCAST_INTERVAL >> 1 >> !pto->verifiedProRegTxHash.IsNull()}); + pto->nNextInvSend = PoissonNextSend(current_time, std::chrono::seconds{INVENTORY_BROADCAST_INTERVAL >> 1 >> !pto->GetVerifiedProRegTxHash().IsNull()}); } } diff --git a/src/rpc/governance.cpp b/src/rpc/governance.cpp index 3107eefd103a..9485b4a17606 100644 --- a/src/rpc/governance.cpp +++ b/src/rpc/governance.cpp @@ -310,11 +310,11 @@ static UniValue gobject_submit(const JSONRPCRequest& request) } auto mnList = deterministicMNManager->GetListAtChainTip(); - bool fMnFound = mnList.HasValidMNByCollateral(activeMasternodeInfo.outpoint); + bool fMnFound = WITH_LOCK(activeMasternodeInfoCs, return mnList.HasValidMNByCollateral(activeMasternodeInfo.outpoint)); LogPrint(BCLog::GOBJECT, "gobject_submit -- pubKeyOperator = %s, outpoint = %s, params.size() = %lld, fMnFound = %d\n", - (activeMasternodeInfo.blsPubKeyOperator ? activeMasternodeInfo.blsPubKeyOperator->ToString() : "N/A"), - activeMasternodeInfo.outpoint.ToStringShort(), request.params.size(), fMnFound); + (WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsPubKeyOperator ? activeMasternodeInfo.blsPubKeyOperator->ToString() : "N/A")), + WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint.ToStringShort()), request.params.size(), fMnFound); // ASSEMBLE NEW GOVERNANCE OBJECT FROM USER PARAMETERS @@ -351,6 +351,7 @@ static UniValue gobject_submit(const JSONRPCRequest& request) // Attempt to sign triggers if we are a MN if (govobj.GetObjectType() == GOVERNANCE_OBJECT_TRIGGER) { if (fMnFound) { + LOCK(activeMasternodeInfoCs); govobj.SetMasternodeOutpoint(activeMasternodeInfo.outpoint); govobj.Sign(*activeMasternodeInfo.blsKeyOperator); } else { @@ -450,7 +451,7 @@ static UniValue gobject_vote_conf(const JSONRPCRequest& request) UniValue statusObj(UniValue::VOBJ); UniValue returnObj(UniValue::VOBJ); - auto dmn = deterministicMNManager->GetListAtChainTip().GetValidMNByCollateral(activeMasternodeInfo.outpoint); + auto dmn = WITH_LOCK(activeMasternodeInfoCs, return deterministicMNManager->GetListAtChainTip().GetValidMNByCollateral(activeMasternodeInfo.outpoint)); if (!dmn) { nFailed++; @@ -468,8 +469,12 @@ static UniValue gobject_vote_conf(const JSONRPCRequest& request) if (govObjType == GOVERNANCE_OBJECT_PROPOSAL && eVoteSignal == VOTE_SIGNAL_FUNDING) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Can't use vote-conf for proposals"); } - if (activeMasternodeInfo.blsKeyOperator) { - signSuccess = vote.Sign(*activeMasternodeInfo.blsKeyOperator); + + { + LOCK(activeMasternodeInfoCs); + if (activeMasternodeInfo.blsKeyOperator) { + signSuccess = vote.Sign(*activeMasternodeInfo.blsKeyOperator); + } } if (!signSuccess) { diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index 006f92309db9..bf3c6882b612 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -233,11 +233,15 @@ static UniValue masternode_status(const JSONRPCRequest& request) UniValue mnObj(UniValue::VOBJ); - // keep compatibility with legacy status for now (might get deprecated/removed later) - mnObj.pushKV("outpoint", activeMasternodeInfo.outpoint.ToStringShort()); - mnObj.pushKV("service", activeMasternodeInfo.service.ToString()); + CDeterministicMNCPtr dmn; + { + LOCK(activeMasternodeInfoCs); - auto dmn = deterministicMNManager->GetListAtChainTip().GetMN(activeMasternodeInfo.proTxHash); + // keep compatibility with legacy status for now (might get deprecated/removed later) + mnObj.pushKV("outpoint", activeMasternodeInfo.outpoint.ToStringShort()); + mnObj.pushKV("service", activeMasternodeInfo.service.ToString()); + dmn = deterministicMNManager->GetListAtChainTip().GetMN(activeMasternodeInfo.proTxHash); + } if (dmn) { mnObj.pushKV("proTxHash", dmn->proTxHash.ToString()); mnObj.pushKV("collateralHash", dmn->collateralOutpoint.hash.ToString()); diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 3ccd94a1ef97..5f6bf5a3787a 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -447,9 +447,8 @@ static UniValue mnauth(const JSONRPCRequest& request) } bool fSuccess = g_connman->ForNode(nodeId, CConnman::AllNodes, [&](CNode* pNode){ - LOCK(pNode->cs_mnauth); - pNode->verifiedProRegTxHash = proTxHash; - pNode->verifiedPubKeyHash = publicKey.GetHash(); + pNode->SetVerifiedProRegTxHash(proTxHash); + pNode->SetVerifiedPubKeyHash(publicKey.GetHash()); return true; }); diff --git a/src/rpc/rpcquorums.cpp b/src/rpc/rpcquorums.cpp index f7fc2f296ec1..a57d3031f3a6 100644 --- a/src/rpc/rpcquorums.cpp +++ b/src/rpc/rpcquorums.cpp @@ -191,6 +191,7 @@ static UniValue quorum_dkgstatus(const JSONRPCRequest& request) } int tipHeight = pindexTip->nHeight; + auto proTxHash = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash); UniValue minableCommitments(UniValue::VOBJ); UniValue quorumConnections(UniValue::VOBJ); for (const auto& type : llmq::CLLMQUtils::GetEnabledQuorumTypes(pindexTip)) { @@ -202,12 +203,13 @@ static UniValue quorum_dkgstatus(const JSONRPCRequest& request) LOCK(cs_main); pindexQuorum = chainActive[tipHeight - (tipHeight % llmq_params.dkgInterval)]; } - auto allConnections = llmq::CLLMQUtils::GetQuorumConnections(llmq_params.type, pindexQuorum, activeMasternodeInfo.proTxHash, false); - auto outboundConnections = llmq::CLLMQUtils::GetQuorumConnections(llmq_params.type, pindexQuorum, activeMasternodeInfo.proTxHash, true); + auto allConnections = llmq::CLLMQUtils::GetQuorumConnections(llmq_params.type, pindexQuorum, proTxHash, false); + auto outboundConnections = llmq::CLLMQUtils::GetQuorumConnections(llmq_params.type, pindexQuorum, proTxHash, true); std::map foundConnections; g_connman->ForEachNode([&](const CNode* pnode) { - if (!pnode->verifiedProRegTxHash.IsNull() && allConnections.count(pnode->verifiedProRegTxHash)) { - foundConnections.emplace(pnode->verifiedProRegTxHash, pnode->addr); + auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); + if (!verifiedProRegTxHash.IsNull() && allConnections.count(verifiedProRegTxHash)) { + foundConnections.emplace(verifiedProRegTxHash, pnode->addr); } }); UniValue arr(UniValue::VARR); diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 7712037eab8c..88cfb1ff7f6d 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -25,7 +25,7 @@ #include static CCriticalSection cs_rpcWarmup; -static bool fRPCRunning = false; +static std::atomic fRPCRunning{false}; static bool fRPCInWarmup GUARDED_BY(cs_rpcWarmup) = true; static std::string rpcWarmupStatus GUARDED_BY(cs_rpcWarmup) = "RPC server started"; /* Timer-creating functions */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a6b987c1c6fd..c6ccd1d203db 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4377,11 +4377,14 @@ bool CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRe void CWallet::KeepKey(int64_t nIndex) { // Remove from key pool - WalletBatch batch(*database); - if (batch.ErasePool(nIndex)) - --nKeysLeftSinceAutoBackup; - if (!nWalletBackups) - nKeysLeftSinceAutoBackup = 0; + { + LOCK(cs_wallet); + WalletBatch batch(*database); + if (batch.ErasePool(nIndex)) + --nKeysLeftSinceAutoBackup; + if (!nWalletBackups) + nKeysLeftSinceAutoBackup = 0; + } WalletLogPrintf("keypool keep %d\n", nIndex); }