Implement checking of open ports and min proto versions in DKGs

src/chainparams.cpp

@@ -122,6 +122,7 @@ void CChainParams::UpdateLLMQTestParams(int size, int threshold) {
     params.size = size;
     params.minSize = threshold;
     params.threshold = threshold;
+    params.dkgBadVotesThreshold = threshold;
 }
 
 void CChainParams::UpdateLLMQDevnetParams(int size, int threshold)
@@ -130,6 +131,7 @@ void CChainParams::UpdateLLMQDevnetParams(int size, int threshold)
     params.size = size;
     params.minSize = threshold;
     params.threshold = threshold;
+    params.dkgBadVotesThreshold = threshold;
 }
 
 static CBlock FindDevNetGenesisBlock(const Consensus::Params& params, const CBlock &prevBlock, const CAmount& reward)
@@ -168,7 +170,7 @@ static Consensus::LLMQParams llmq_test = {
         .dkgPhaseBlocks = 2,
         .dkgMiningWindowStart = 10, // dkgPhaseBlocks * 5 = after finalization
         .dkgMiningWindowEnd = 18,
-        .dkgBadVotesThreshold = 8,
+        .dkgBadVotesThreshold = 2,
 
         .signingActiveQuorumCount = 2, // just a few ones to allow easier testing
 
@@ -726,6 +728,7 @@ class CDevNetParams : public CChainParams {
 
         fDefaultConsistencyChecks = false;
         fRequireStandard = false;
+        fRequireRoutableExternalIP = true;
         fMineBlocksOnDemand = false;
         fAllowMultipleAddressesFromGroup = true;
         fAllowMultiplePorts = true;

src/evo/providertx.cpp

@@ -23,7 +23,7 @@ static bool CheckService(const uint256& proTxHash, const ProTx& proTx, CValidati
     if (!proTx.addr.IsValid()) {
         return state.DoS(10, false, REJECT_INVALID, "bad-protx-ipaddr");
     }
-    if (Params().NetworkIDString() != CBaseChainParams::REGTEST && !proTx.addr.IsRoutable()) {
+    if (Params().RequireRoutableExternalIP() && !proTx.addr.IsRoutable()) {
         return state.DoS(10, false, REJECT_INVALID, "bad-protx-ipaddr");
     }
 

src/init.cpp

@@ -1459,7 +1459,7 @@ bool AppInitParameterInteraction()
     }
 
     if (gArgs.IsArgSet("-masternodeblsprivkey")) {
-        if (!gArgs.GetBoolArg("-listen", DEFAULT_LISTEN)) {
+        if (!gArgs.GetBoolArg("-listen", DEFAULT_LISTEN) && Params().RequireRoutableExternalIP()) {
             return InitError("Masternode must accept connections from outside, set -listen=1");
         }
         if (!gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {

src/llmq/quorums_dkgsession.cpp

@@ -12,6 +12,7 @@
 #include <evo/specialtx.h>
 
 #include <masternode/activemasternode.h>
+#include <masternode/masternode-meta.h>
 #include <chainparams.h>
 #include <init.h>
 #include <net.h>
@@ -437,9 +438,49 @@ void CDKGSession::VerifyAndComplain(CDKGPendingMessages& pendingMessages)
     logger.Batch("verified contributions. time=%d", t1.count());
     logger.Flush();
 
+    VerifyConnectionAndMinProtoVersions();
+
     SendComplaint(pendingMessages);
 }
 
+void CDKGSession::VerifyConnectionAndMinProtoVersions()
+{
+    if (!sporkManager.IsSporkActive(SPORK_21_QUORUM_ALL_CONNECTED)) {
+        return;
+    }
+
+    CDKGLogger logger(*this, __func__);
+
+    std::unordered_map<uint256, int, StaticSaltedHasher> protoMap;
+    g_connman->ForEachNode([&](const CNode* pnode) {
+        if (pnode->verifiedProRegTxHash.IsNull()) {
+            return;
+        }
+        protoMap.emplace(pnode->verifiedProRegTxHash, pnode->nVersion);
+    });
+
+    for (auto& m : members) {
+        if (m->dmn->proTxHash == myProTxHash) {
+            continue;
+        }
+
+        auto it = protoMap.find(m->dmn->proTxHash);
+        if (it == protoMap.end()) {
+            m->badConnection = true;
+            logger.Batch("%s is not connected to us", m->dmn->proTxHash.ToString());
+        } else if (it != protoMap.end() && it->second < MIN_MASTERNODE_PROTO_VERSION) {
+            m->badConnection = true;
+            logger.Batch("%s does not have min proto version %d (has %d)", m->dmn->proTxHash.ToString(), MIN_MASTERNODE_PROTO_VERSION, it->second);
+        }
+
+        auto lastOutbound = mmetaman.GetMetaInfo(m->dmn->proTxHash)->GetLastOutboundSuccess();
+        if (GetAdjustedTime() - lastOutbound > 60 * 60) {
+            m->badConnection = true;
+            logger.Batch("%s no outbound connection since %d seconds", m->dmn->proTxHash.ToString(), GetAdjustedTime() - lastOutbound);
+        }
+    }
+}
+
 void CDKGSession::SendComplaint(CDKGPendingMessages& pendingMessages)
 {
     CDKGLogger logger(*this, __func__);
@@ -455,7 +496,7 @@ void CDKGSession::SendComplaint(CDKGPendingMessages& pendingMessages)
     int complaintCount = 0;
     for (size_t i = 0; i < members.size(); i++) {
         auto& m = members[i];
-        if (m->bad) {
+        if (m->bad || m->badConnection) {
             qc.badMembers[i] = true;
             badCount++;
         } else if (m->weComplain) {

src/llmq/quorums_dkgsession.h

@@ -217,6 +217,7 @@ class CDKGMember
     std::set<uint256> complaintsFromOthers;
 
     bool bad{false};
+    bool badConnection{false};
     bool weComplain{false};
     bool someoneComplain{false};
 };
@@ -310,6 +311,7 @@ class CDKGSession
 
     // Phase 2: complaint
     void VerifyAndComplain(CDKGPendingMessages& pendingMessages);
+    void VerifyConnectionAndMinProtoVersions();
     void SendComplaint(CDKGPendingMessages& pendingMessages);
     bool PreVerifyMessage(const uint256& hash, const CDKGComplaint& qc, bool& retBan) const;
     void ReceiveMessage(const uint256& hash, const CDKGComplaint& qc, bool& retBan);

src/masternode/activemasternode.cpp

@@ -68,7 +68,7 @@ void CActiveMasternodeManager::Init(const CBlockIndex* pindex)
     if (!deterministicMNManager->IsDIP3Enforced(pindex->nHeight)) return;
 
     // Check that our local network configuration is correct
-    if (!fListen) {
+    if (!fListen && Params().RequireRoutableExternalIP()) {
         // listen option is probably overwritten by something else, no good
         state = MASTERNODE_ERROR;
         strError = "Masternode must accept connections from outside. Make sure listen configuration option is not overwritten by some another parameter.";
@@ -119,7 +119,7 @@ void CActiveMasternodeManager::Init(const CBlockIndex* pindex)
     bool fConnected = ConnectSocketDirectly(activeMasternodeInfo.service, hSocket, nConnectTimeout) && IsSelectableSocket(hSocket);
     CloseSocket(hSocket);
 
-    if (!fConnected) {
+    if (!fConnected && Params().RequireRoutableExternalIP()) {
         state = MASTERNODE_ERROR;
         strError = "Could not connect to " + activeMasternodeInfo.service.ToString();
         LogPrintf("CActiveMasternodeManager::Init -- ERROR: %s\n", strError);
@@ -190,7 +190,7 @@ bool CActiveMasternodeManager::GetLocalAddress(CService& addrRet)
     if (LookupHost("8.8.8.8", addrDummyPeer, false)) {
         fFoundLocal = GetLocal(addrRet, &addrDummyPeer) && IsValidNetAddr(addrRet);
     }
-    if (!fFoundLocal && Params().NetworkIDString() == CBaseChainParams::REGTEST) {
+    if (!fFoundLocal && !Params().RequireRoutableExternalIP()) {
         if (Lookup("127.0.0.1", addrRet, GetListenPort(), false)) {
             fFoundLocal = true;
         }
@@ -218,6 +218,6 @@ bool CActiveMasternodeManager::IsValidNetAddr(CService addrIn)
 {
     // TODO: regtest is fine with any addresses for now,
     // should probably be a bit smarter if one day we start to implement tests for this
-    return Params().NetworkIDString() == CBaseChainParams::REGTEST ||
+    return !Params().RequireRoutableExternalIP() ||
            (addrIn.IsIPv4() && IsReachable(addrIn) && addrIn.IsRoutable());
 }

src/masternode/masternode-sync.cpp

@@ -115,7 +115,7 @@ void CMasternodeSync::ProcessTick(CConnman& connman)
 
     // reset the sync process if the last call to this function was more than 60 minutes ago (client was in sleep mode)
     static int64_t nTimeLastProcess = GetTime();
-    if(GetTime() - nTimeLastProcess > 60*60) {
+    if(GetTime() - nTimeLastProcess > 60*60 && !fMasternodeMode) {
         LogPrintf("CMasternodeSync::ProcessTick -- WARNING: no actions for too long, restarting sync...\n");
         Reset();
         SwitchToNextAsset(connman);

src/net.cpp

@@ -2197,7 +2197,7 @@ void CConnman::ThreadOpenMasternodeConnections()
 
                     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);
@@ -2285,6 +2285,12 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
     {
         LOCK(cs_vNodes);
         vNodes.push_back(pnode);
+
+        if (!fNetworkActive) {
+            // there is a small chance of fNetworkActive becoming false between the start of this method
+            // and the successful lock of cs_vNodes
+            pnode->CloseSocketDisconnect();
+        }
     }
 }
 

src/net_processing.cpp

@@ -304,6 +304,8 @@ void UpdatePreferredDownload(CNode* node, CNodeState* state)
 
 void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime)
 {
+    const auto& params = Params();
+
     ServiceFlags nLocalNodeServices = pnode->GetLocalServices();
     uint64_t nonce = pnode->GetLocalNonce();
     int nNodeStartingHeight = pnode->GetMyStartingHeight();
@@ -320,13 +322,18 @@ void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime)
         pnode->sentMNAuthChallenge = mnauthChallenge;
     }
 
-    connman->PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, (uint64_t)nLocalNodeServices, nTime, addrYou, addrMe,
+    int nProtocolVersion = PROTOCOL_VERSION;
+    if (params.NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
+        nProtocolVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
+    }
+
+    connman->PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, nProtocolVersion, (uint64_t)nLocalNodeServices, nTime, addrYou, addrMe,
             nonce, strSubVersion, nNodeStartingHeight, ::fRelayTxes, mnauthChallenge, pnode->fMasternode));
 
     if (fLogIPs) {
-        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, them=%s, peer=%d\n", PROTOCOL_VERSION, nNodeStartingHeight, addrMe.ToString(), addrYou.ToString(), nodeid);
+        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, them=%s, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), addrYou.ToString(), nodeid);
     } else {
-        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, peer=%d\n", PROTOCOL_VERSION, nNodeStartingHeight, addrMe.ToString(), nodeid);
+        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), nodeid);
     }
 }
 

src/version.h

@@ -22,6 +22,9 @@ static const int GETHEADERS_VERSION = 70077;
 //! disconnect from peers older than this proto version
 static const int MIN_PEER_PROTO_VERSION = 70213;
 
+//! minimum proto version of masternode to accept in DKGs
+static const int MIN_MASTERNODE_PROTO_VERSION = 70217;
+
 //! nTime field added to CAddress, starting with this version;
 //! if possible, avoid requesting addresses nodes older than this
 static const int CADDR_TIME_VERSION = 31402;

test/functional/llmq-simplepose.py

@@ -26,24 +26,104 @@ def run_test(self):
         self.wait_for_sporks_same()
 
         # check if mining quorums with all nodes being online succeeds without punishment/banning
+        self.test_no_banning()
+
+        # Now lets isolate MNs one by one and verify that punishment/banning happens
+        def isolate_mn(mn):
+            mn.node.setnetworkactive(False)
+            wait_until(lambda: mn.node.getconnectioncount() == 0)
+        self.test_banning(isolate_mn, False, True, False)
+
+        self.repair_masternodes(False)
+
+        self.nodes[0].spork("SPORK_21_QUORUM_ALL_CONNECTED", 0)
+        self.wait_for_sporks_same()
+
+        self.reset_probe_timeouts()
+
+        # Make sure no banning happens with spork21 enabled
+        self.test_no_banning(expected_connections=4, expected_probes=4)
+
+        # Lets restart masternodes with closed ports and verify that they get banned even though they are connected to other MNs (via outbound connections)
+        def close_mn_port(mn):
+            self.stop_node(mn.node.index)
+            self.start_masternode(mn, ["-listen=0", "-mocktime=%d" % self.mocktime])
+            connect_nodes(mn.node, 0)
+            # Make sure the to-be-banned node is still connected well via outbound connections
+            for mn2 in self.mninfo:
+                if mn2 is not mn:
+                    connect_nodes(mn.node, mn2.node.index)
+            self.reset_probe_timeouts()
+        self.test_banning(close_mn_port, True, False, True)
+
+        self.repair_masternodes(True)
+        self.reset_probe_timeouts()
+
+        def force_old_mn_proto(mn):
+            self.stop_node(mn.node.index)
+            self.start_masternode(mn, ["-pushversion=70216", "-mocktime=%d" % self.mocktime])
+            connect_nodes(mn.node, 0)
+            self.reset_probe_timeouts()
+        self.test_banning(force_old_mn_proto, True, False, False)
+
+    def test_no_banning(self, expected_connections=1, expected_probes=0):
         for i in range(3):
-            self.mine_quorum()
+            self.mine_quorum(expected_connections=expected_connections, expected_probes=expected_probes)
         for mn in self.mninfo:
-            assert(not self.check_punished(mn) and not self.check_punished(mn))
+            assert(not self.check_punished(mn) and not self.check_banned(mn))
 
-        # Now lets kill MNs one by one and verify that punishment/banning happens
-        for i in range(len(self.mninfo), len(self.mninfo) - 2, -1):
-            mn = self.mninfo[len(self.mninfo) - 1]
-            self.mninfo.remove(mn)
-            self.stop_node(mn.nodeIdx)
-            self.nodes.remove(mn.node)
+    def test_banning(self, invalidate_proc, check_probes, expect_contribution_to_fail, expect_probes_to_fail):
+        online_mninfos = self.mninfo.copy()
+        for i in range(2):
+            mn = online_mninfos[len(online_mninfos) - 1]
+            online_mninfos.remove(mn)
+            invalidate_proc(mn)
 
             t = time.time()
             while (not self.check_punished(mn) or not self.check_banned(mn)) and (time.time() - t) < 120:
-                self.mine_quorum(expected_connections=1, expected_members=i-1, expected_contributions=i-1, expected_complaints=i-1, expected_commitments=i-1)
+                expected_probes = 0
+                expected_contributors = len(online_mninfos) + 1
+                if check_probes:
+                    expected_probes = len(online_mninfos)
+                    if expect_probes_to_fail:
+                        expected_probes -= 1
+                if expect_contribution_to_fail:
+                    expected_contributors -= 1
+                self.mine_quorum(expected_connections=1, expected_probes=expected_probes, expected_members=len(online_mninfos), expected_contributions=expected_contributors, expected_complaints=expected_contributors-1, expected_commitments=expected_contributors, mninfos=online_mninfos)
 
             assert(self.check_punished(mn) and self.check_banned(mn))
 
+    def repair_masternodes(self, restart):
+        # Repair all nodes
+        for mn in self.mninfo:
+            if self.check_banned(mn) or self.check_punished(mn):
+                addr = self.nodes[0].getnewaddress()
+                self.nodes[0].sendtoaddress(addr, 0.1)
+                self.nodes[0].protx('update_service', mn.proTxHash, '127.0.0.1:%d' % p2p_port(mn.node.index), mn.keyOperator, "", addr)
+                self.nodes[0].generate(1)
+                assert(not self.check_banned(mn))
+
+                if restart:
+                    self.stop_node(mn.node.index)
+                    self.start_masternode(mn, ["-mocktime=%d" % self.mocktime])
+                else:
+                    mn.node.setnetworkactive(True)
+            connect_nodes(mn.node, 0)
+        self.sync_all()
+
+        # Isolate and re-connect all MNs (otherwise there might be open connections with no MNAUTH for MNs which were banned before)
+        for mn in self.mninfo:
+            mn.node.setnetworkactive(False)
+            wait_until(lambda: mn.node.getconnectioncount() == 0)
+            mn.node.setnetworkactive(True)
+            connect_nodes(mn.node, 0)
+
+    def reset_probe_timeouts(self):
+        # Make sure all masternodes will reconnect/re-probe
+        self.bump_mocktime(60 * 60 + 1)
+        set_node_times(self.nodes, self.mocktime)
+        self.sync_all()
+
     def check_punished(self, mn):
         info = self.nodes[0].protx('info', mn.proTxHash)
         if info['state']['PoSePenalty'] > 0: