From 0adef2cf7a2ba0ec64924e84bf2126cf20fa89e6 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 17 Mar 2020 08:36:45 +0100 Subject: [PATCH 1/6] Fix ThreadOpenMasternodeConnections to not drop pending MN connections The way it was implemented caused vPendingMasternodes entries to be popped but not necessarily connected to when at the same time quorum connections were pending. --- src/net.cpp | 54 +++++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 09a729c0ca..59565ba5ca 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2130,40 +2130,42 @@ void CConnman::ThreadOpenMasternodeConnections() { // don't hold lock while calling OpenMasternodeConnection as cs_main is locked deep inside LOCK2(cs_vNodes, cs_vPendingMasternodes); - std::vector pending; - for (const auto& group : masternodeQuorumNodes) { - for (const auto& proRegTxHash : group.second) { - auto dmn = mnList.GetMN(proRegTxHash); - if (!dmn) { - continue; - } - const auto& addr2 = dmn->pdmnState->addr; - if (!connectedNodes.count(addr2) && !IsMasternodeOrDisconnectRequested(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { - auto addrInfo = addrman.GetAddressInfo(addr2); - // back off trying connecting to an address if we already tried recently - if (addrInfo.IsValid() && nANow - addrInfo.nLastTry < 60) { - continue; - } - pending.emplace_back(addr2); - } - } - } - if (!vPendingMasternodes.empty()) { auto addr2 = vPendingMasternodes.front(); vPendingMasternodes.erase(vPendingMasternodes.begin()); if (!connectedNodes.count(addr2) && !IsMasternodeOrDisconnectRequested(addr2)) { - pending.emplace_back(addr2); + addr = addr2; } } - if (pending.empty()) { - // nothing to do, keep waiting - continue; - } + if (addr == CService()) { + std::vector pending; + for (const auto& group : masternodeQuorumNodes) { + for (const auto& proRegTxHash : group.second) { + auto dmn = mnList.GetMN(proRegTxHash); + if (!dmn) { + continue; + } + const auto& addr2 = dmn->pdmnState->addr; + if (!connectedNodes.count(addr2) && !IsMasternodeOrDisconnectRequested(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { + auto addrInfo = addrman.GetAddressInfo(addr2); + // back off trying connecting to an address if we already tried recently + if (addrInfo.IsValid() && nANow - addrInfo.nLastTry < 60) { + continue; + } + pending.emplace_back(addr2); + } + } + } - std::random_shuffle(pending.begin(), pending.end()); - addr = pending.front(); + if (!pending.empty()) { + addr = pending[GetRandInt(pending.size())]; + } + } + } + + if (addr == CService()) { + continue; } OpenMasternodeConnection(CAddress(addr, NODE_NETWORK)); From 35d75b19e6235460d7f3ab18acf082f6dde47066 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Thu, 19 Mar 2020 14:21:02 +0100 Subject: [PATCH 2/6] Make pending masternode queue proTxHash based Instead of CService --- src/net.cpp | 31 +++++++++++++------------- src/net.h | 4 ++-- src/privatesend/privatesend-client.cpp | 4 ++-- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 59565ba5ca..3b08781408 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2126,20 +2126,20 @@ void CConnman::ThreadOpenMasternodeConnections() // NOTE: Process only one pending masternode at a time - CService addr; + CDeterministicMNCPtr connectToDmn; { // don't hold lock while calling OpenMasternodeConnection as cs_main is locked deep inside LOCK2(cs_vNodes, cs_vPendingMasternodes); if (!vPendingMasternodes.empty()) { - auto addr2 = vPendingMasternodes.front(); + auto dmn = mnList.GetValidMN(vPendingMasternodes.front()); vPendingMasternodes.erase(vPendingMasternodes.begin()); - if (!connectedNodes.count(addr2) && !IsMasternodeOrDisconnectRequested(addr2)) { - addr = addr2; + if (dmn && !connectedNodes.count(dmn->pdmnState->addr) && !IsMasternodeOrDisconnectRequested(dmn->pdmnState->addr)) { + connectToDmn = dmn; } } - if (addr == CService()) { - std::vector pending; + if (!connectToDmn) { + std::vector pending; for (const auto& group : masternodeQuorumNodes) { for (const auto& proRegTxHash : group.second) { auto dmn = mnList.GetMN(proRegTxHash); @@ -2153,24 +2153,24 @@ void CConnman::ThreadOpenMasternodeConnections() if (addrInfo.IsValid() && nANow - addrInfo.nLastTry < 60) { continue; } - pending.emplace_back(addr2); + pending.emplace_back(dmn); } } } if (!pending.empty()) { - addr = pending[GetRandInt(pending.size())]; + connectToDmn = pending[GetRandInt(pending.size())]; } } } - if (addr == CService()) { + if (!connectToDmn) { continue; } - OpenMasternodeConnection(CAddress(addr, NODE_NETWORK)); + OpenMasternodeConnection(CAddress(connectToDmn->pdmnState->addr, NODE_NETWORK)); // should be in the list now if connection was opened - ForNode(addr, CConnman::AllNodes, [&](CNode* pnode) { + ForNode(connectToDmn->pdmnState->addr, CConnman::AllNodes, [&](CNode* pnode) { if (pnode->fDisconnect) { return false; } @@ -2782,15 +2782,14 @@ bool CConnman::RemoveAddedNode(const std::string& strNode) return false; } -bool CConnman::AddPendingMasternode(const CService& service) +bool CConnman::AddPendingMasternode(const uint256& proTxHash) { LOCK(cs_vPendingMasternodes); - for(const auto& s : vPendingMasternodes) { - if (service == s) - return false; + if (std::find(vPendingMasternodes.begin(), vPendingMasternodes.end(), proTxHash) != vPendingMasternodes.end()) { + return false; } - vPendingMasternodes.push_back(service); + vPendingMasternodes.push_back(proTxHash); return true; } diff --git a/src/net.h b/src/net.h index 5e05078589..b7b35a7ffb 100644 --- a/src/net.h +++ b/src/net.h @@ -407,7 +407,7 @@ public: bool RemoveAddedNode(const std::string& node); std::vector GetAddedNodeInfo(); - bool AddPendingMasternode(const CService& addr); + bool AddPendingMasternode(const uint256& proTxHash); bool AddMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set& proTxHashes); bool HasMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash); std::set GetMasternodeQuorums(Consensus::LLMQType llmqType); @@ -543,7 +543,7 @@ private: CCriticalSection cs_vOneShots; std::vector vAddedNodes GUARDED_BY(cs_vAddedNodes); CCriticalSection cs_vAddedNodes; - std::vector vPendingMasternodes; + std::vector vPendingMasternodes; std::map, std::set> masternodeQuorumNodes; // protected by cs_vPendingMasternodes mutable CCriticalSection cs_vPendingMasternodes; std::vector vNodes; diff --git a/src/privatesend/privatesend-client.cpp b/src/privatesend/privatesend-client.cpp index fa38003257..5356e3196a 100644 --- a/src/privatesend/privatesend-client.cpp +++ b/src/privatesend/privatesend-client.cpp @@ -1081,7 +1081,7 @@ bool CPrivateSendClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymize nSessionDenom = dsq.nDenom; mixingMasternode = dmn; pendingDsaRequest = CPendingDsaRequest(dmn->pdmnState->addr, CPrivateSendAccept(nSessionDenom, txMyCollateral)); - connman.AddPendingMasternode(dmn->pdmnState->addr); + connman.AddPendingMasternode(dmn->proTxHash); // TODO: add new state POOL_STATE_CONNECTING and bump MIN_PRIVATESEND_PEER_PROTO_VERSION SetState(POOL_STATE_QUEUE); nTimeLastSuccessfulStep = GetTime(); @@ -1158,7 +1158,7 @@ bool CPrivateSendClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, C } mixingMasternode = dmn; - connman.AddPendingMasternode(dmn->pdmnState->addr); + connman.AddPendingMasternode(dmn->proTxHash); pendingDsaRequest = CPendingDsaRequest(dmn->pdmnState->addr, CPrivateSendAccept(nSessionDenom, txMyCollateral)); // TODO: add new state POOL_STATE_CONNECTING and bump MIN_PRIVATESEND_PEER_PROTO_VERSION SetState(POOL_STATE_QUEUE); From 93ed22b239212feb10148c82034eeaba7490524c Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Thu, 19 Mar 2020 14:21:06 +0100 Subject: [PATCH 3/6] Logging for outgoing masternode connections --- src/net.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index 3b08781408..ed8ee73744 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2135,6 +2135,7 @@ void CConnman::ThreadOpenMasternodeConnections() vPendingMasternodes.erase(vPendingMasternodes.begin()); if (dmn && !connectedNodes.count(dmn->pdmnState->addr) && !IsMasternodeOrDisconnectRequested(dmn->pdmnState->addr)) { connectToDmn = dmn; + LogPrint(BCLog::NET, "CConnman::%s -- opening pending masternode connection to %s, service=%s\n", __func__, dmn->proTxHash.ToString(), dmn->pdmnState->addr.ToString(false)); } } @@ -2160,6 +2161,7 @@ void CConnman::ThreadOpenMasternodeConnections() if (!pending.empty()) { connectToDmn = pending[GetRandInt(pending.size())]; + LogPrint(BCLog::NET, "CConnman::%s -- opening quorum connection to %s, service=%s\n", __func__, connectToDmn->proTxHash.ToString(), connectToDmn->pdmnState->addr.ToString(false)); } } } From 458a63736d6f38fc7b69c04f5b14f5fa850d007e Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Thu, 19 Mar 2020 13:51:25 +0100 Subject: [PATCH 4/6] Track last outbound connection attempts in CMasternodeMetaMan Instead of relying on CAddrMan, which only works well for addresses announced in P2P networking (and not with MNs). --- src/masternode/masternode-meta.cpp | 2 +- src/masternode/masternode-meta.h | 9 ++++++++- src/net.cpp | 7 +++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/masternode/masternode-meta.cpp b/src/masternode/masternode-meta.cpp index 8b5e238c80..6889f89008 100644 --- a/src/masternode/masternode-meta.cpp +++ b/src/masternode/masternode-meta.cpp @@ -6,7 +6,7 @@ CMasternodeMetaMan mmetaman; -const std::string CMasternodeMetaMan::SERIALIZATION_VERSION_STRING = "CMasternodeMetaMan-Version-1"; +const std::string CMasternodeMetaMan::SERIALIZATION_VERSION_STRING = "CMasternodeMetaMan-Version-2"; void CMasternodeMetaInfo::AddGovernanceVote(const uint256& nGovernanceObjectHash) { diff --git a/src/masternode/masternode-meta.h b/src/masternode/masternode-meta.h index 9b33f56c6f..07cc0d0a8e 100644 --- a/src/masternode/masternode-meta.h +++ b/src/masternode/masternode-meta.h @@ -33,6 +33,8 @@ private: // KEEP TRACK OF GOVERNANCE ITEMS EACH MASTERNODE HAS VOTE UPON FOR RECALCULATION std::map mapGovernanceObjectsVotedOn; + int64_t lastOutboundAttempt = 0; + public: CMasternodeMetaInfo() {} explicit CMasternodeMetaInfo(const uint256& _proTxHash) : proTxHash(_proTxHash) {} @@ -40,7 +42,8 @@ public: proTxHash(ref.proTxHash), nLastDsq(ref.nLastDsq), nMixingTxCount(ref.nMixingTxCount), - mapGovernanceObjectsVotedOn(ref.mapGovernanceObjectsVotedOn) + mapGovernanceObjectsVotedOn(ref.mapGovernanceObjectsVotedOn), + lastOutboundAttempt(ref.lastOutboundAttempt) { } @@ -53,6 +56,7 @@ public: READWRITE(nLastDsq); READWRITE(nMixingTxCount); READWRITE(mapGovernanceObjectsVotedOn); + READWRITE(lastOutboundAttempt); } public: @@ -66,6 +70,9 @@ public: void AddGovernanceVote(const uint256& nGovernanceObjectHash); void RemoveGovernanceObject(const uint256& nGovernanceObjectHash); + + void SetLastOutboundAttempt(int64_t t) { LOCK(cs); lastOutboundAttempt = t; } + int64_t GetLastOutboundAttempt() const { LOCK(cs); return lastOutboundAttempt; } }; typedef std::shared_ptr CMasternodeMetaInfoPtr; diff --git a/src/net.cpp b/src/net.cpp index ed8ee73744..b5ffe20c01 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -2149,9 +2150,9 @@ void CConnman::ThreadOpenMasternodeConnections() } const auto& addr2 = dmn->pdmnState->addr; if (!connectedNodes.count(addr2) && !IsMasternodeOrDisconnectRequested(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { - auto addrInfo = addrman.GetAddressInfo(addr2); + int64_t lastAttempt = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastOutboundAttempt(); // back off trying connecting to an address if we already tried recently - if (addrInfo.IsValid() && nANow - addrInfo.nLastTry < 60) { + if (nANow - lastAttempt < 60) { continue; } pending.emplace_back(dmn); @@ -2170,6 +2171,8 @@ void CConnman::ThreadOpenMasternodeConnections() continue; } + mmetaman.GetMetaInfo(connectToDmn->proTxHash)->SetLastOutboundAttempt(nANow); + OpenMasternodeConnection(CAddress(connectToDmn->pdmnState->addr, NODE_NETWORK)); // should be in the list now if connection was opened ForNode(connectToDmn->pdmnState->addr, CConnman::AllNodes, [&](CNode* pnode) { From 2a6465a6fb286f9a6d628e5d96cb962523060592 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Sat, 21 Mar 2020 12:21:09 +0100 Subject: [PATCH 5/6] Move LLMQ connection retry timeout into chainparams --- src/chainparams.cpp | 4 ++++ src/chainparams.h | 3 +++ src/net.cpp | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 01017fb8a3..81d8f083a1 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -395,6 +395,7 @@ public: fMineBlocksOnDemand = false; fAllowMultipleAddressesFromGroup = false; fAllowMultiplePorts = false; + nLLMQConnectionRetryTimeout = 60; nPoolMinParticipants = 3; nPoolMaxParticipants = 5; // TODO: bump on next HF / mandatory upgrade @@ -571,6 +572,7 @@ public: fMineBlocksOnDemand = false; fAllowMultipleAddressesFromGroup = false; fAllowMultiplePorts = true; + nLLMQConnectionRetryTimeout = 60; nPoolMinParticipants = 3; // TODO drop to 2 with next mandatory upgrade nPoolMaxParticipants = 5; @@ -727,6 +729,7 @@ public: fMineBlocksOnDemand = false; fAllowMultipleAddressesFromGroup = true; fAllowMultiplePorts = true; + nLLMQConnectionRetryTimeout = 60; nPoolMinParticipants = 3; // same, drop to 2 w/ breaking change nPoolMaxParticipants = 5; @@ -837,6 +840,7 @@ public: fMineBlocksOnDemand = true; fAllowMultipleAddressesFromGroup = true; fAllowMultiplePorts = true; + nLLMQConnectionRetryTimeout = 30; // must be lower then the LLMQ signing session timeout so that tests have control over failing behavior nFulfilledRequestExpireTime = 5*60; // fulfilled requests expire in 5 minutes nPoolMinParticipants = 2; diff --git a/src/chainparams.h b/src/chainparams.h index c383ad2b4c..aaae1262e0 100644 --- a/src/chainparams.h +++ b/src/chainparams.h @@ -70,6 +70,8 @@ public: bool AllowMultipleAddressesFromGroup() const { return fAllowMultipleAddressesFromGroup; } /** Allow nodes with the same address and multiple ports */ bool AllowMultiplePorts() const { return fAllowMultiplePorts; } + /** How long to wait until we allow retrying of a LLMQ connection */ + int LLMQConnectionRetryTimeout() const { return nLLMQConnectionRetryTimeout; } /** Return the BIP70 network string (main, test or regtest) */ std::string NetworkIDString() const { return strNetworkID; } /** Return the list of hostnames to look up for DNS seeds */ @@ -112,6 +114,7 @@ protected: bool fMineBlocksOnDemand; bool fAllowMultipleAddressesFromGroup; bool fAllowMultiplePorts; + int nLLMQConnectionRetryTimeout; CCheckpointData checkpointData; ChainTxData chainTxData; int nPoolMinParticipants; diff --git a/src/net.cpp b/src/net.cpp index b5ffe20c01..5218783b9c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2103,6 +2103,8 @@ void CConnman::ThreadOpenMasternodeConnections() if (gArgs.IsArgSet("-connect") && gArgs.GetArgs("-connect").size() > 0) return; + auto& chainParams = Params(); + while (!interruptNet) { if (!interruptNet.sleep_for(std::chrono::milliseconds(1000))) @@ -2152,7 +2154,7 @@ void CConnman::ThreadOpenMasternodeConnections() if (!connectedNodes.count(addr2) && !IsMasternodeOrDisconnectRequested(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { int64_t lastAttempt = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastOutboundAttempt(); // back off trying connecting to an address if we already tried recently - if (nANow - lastAttempt < 60) { + if (nANow - lastAttempt < chainParams.LLMQConnectionRetryTimeout()) { continue; } pending.emplace_back(dmn); From 51dda92a12e7a7da32eb81ad111530f9b228ffec Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Sat, 21 Mar 2020 12:21:25 +0100 Subject: [PATCH 6/6] Bump mocktime after reconnecting nodes --- test/functional/llmq-is-retroactive.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/functional/llmq-is-retroactive.py b/test/functional/llmq-is-retroactive.py index 8dede9da9b..f8a4fcd996 100755 --- a/test/functional/llmq-is-retroactive.py +++ b/test/functional/llmq-is-retroactive.py @@ -60,6 +60,9 @@ class LLMQ_IS_RetroactiveSigning(DashTestFramework): self.wait_for_tx(txid, self.nodes[1]) self.wait_for_tx(txid, self.nodes[2]) reconnect_isolated_node(self.nodes[3], 0) + # Make sure nodes actually try re-connecting quorum connections + self.bump_mocktime(30) + set_node_times(self.nodes, self.mocktime) self.wait_for_mnauth(self.nodes[3], 2) # node 3 fully reconnected but the TX wasn't relayed to it, so there should be no IS lock self.wait_for_instantlock(txid, self.nodes[0], False, 5) @@ -91,6 +94,9 @@ class LLMQ_IS_RetroactiveSigning(DashTestFramework): self.wait_for_tx(txid, self.nodes[1]) self.wait_for_tx(txid, self.nodes[2]) reconnect_isolated_node(self.nodes[3], 0) + # Make sure nodes actually try re-connecting quorum connections + self.bump_mocktime(30) + set_node_times(self.nodes, self.mocktime) self.wait_for_mnauth(self.nodes[3], 2) # node 3 fully reconnected but the TX wasn't relayed to it, so there should be no IS lock self.wait_for_instantlock(txid, self.nodes[0], False, 5) @@ -133,6 +139,9 @@ class LLMQ_IS_RetroactiveSigning(DashTestFramework): set_node_times(self.nodes, self.mocktime) time.sleep(2) # make sure Cleanup() is called reconnect_isolated_node(self.nodes[3], 0) + # Make sure nodes actually try re-connecting quorum connections + self.bump_mocktime(30) + set_node_times(self.nodes, self.mocktime) self.wait_for_mnauth(self.nodes[3], 2) # node 3 fully reconnected but the signing session is already timed out on all nodes, so no IS lock self.wait_for_instantlock(txid, self.nodes[0], False, 5) @@ -158,6 +167,9 @@ class LLMQ_IS_RetroactiveSigning(DashTestFramework): set_node_times(self.nodes, self.mocktime) time.sleep(2) # make sure Cleanup() is called reconnect_isolated_node(self.nodes[3], 0) + # Make sure nodes actually try re-connecting quorum connections + self.bump_mocktime(30) + set_node_times(self.nodes, self.mocktime) self.wait_for_mnauth(self.nodes[3], 2) self.nodes[0].sendrawtransaction(rawtx) # Make sure nodes 1 and 2 received the TX