Merge bitcoin#31157: Cleanups to port mapping module post UPnP drop

70398ae mapport: make ProcessPCP void (Antoine Poinsot)
9e6cba2 mapport: remove unnecessary 'g_mapport_enabled' (Antoine Poinsot)
8fb45fc mapport: remove unnecessary 'g_mapport_current' variable (Antoine Poinsot)
1b223cb mapport: merge DispatchMapPort into StartMapPort (Antoine Poinsot)
9bd936f mapport: drop unnecessary function (Antoine Poinsot)
2a6536c mapport: rename 'use_pcp' to 'enable' (Antoine Poinsot)
c4e82b8 mapport: make 'enabled' and 'current' bool (Antoine Poinsot)

Pull request description:

  Followup to bitcoin#31130, this does a couple cleanups to `src/mapport.*` to clarify the logic now that there is a single protocol option for port mapping.

ACKs for top commit:
  laanwj:
    Code review ACK 70398ae
  TheCharlatan:
    ACK 70398ae

Tree-SHA512: d9a3ab4fcd59a7cf4872415c40cc7ac3a98dfc5aa25e195d4df880bb588bac286c30c3471e9d9499de379a75f45dcd0a82019eba3cb9f342004ae1482d0ba075

Author: fanquake

src/interfaces/node.h

@@ -121,7 +121,7 @@ class Node
     virtual void resetSettings() = 0;
 
     //! Map port.
-    virtual void mapPort(bool use_pcp) = 0;
+    virtual void mapPort(bool enable) = 0;
 
     //! Get proxy.
     virtual bool getProxy(Network net, Proxy& proxy_info) = 0;

src/mapport.cpp

@@ -25,14 +25,12 @@
 
 static CThreadInterrupt g_mapport_interrupt;
 static std::thread g_mapport_thread;
-static std::atomic_uint g_mapport_enabled_protos{MapPortProtoFlag::NONE};
-static std::atomic<MapPortProtoFlag> g_mapport_current_proto{MapPortProtoFlag::NONE};
 
 using namespace std::chrono_literals;
 static constexpr auto PORT_MAPPING_REANNOUNCE_PERIOD{20min};
 static constexpr auto PORT_MAPPING_RETRY_PERIOD{5min};
 
-static bool ProcessPCP()
+static void ProcessPCP()
 {
     // The same nonce is used for all mappings, this is allowed by the spec, and simplifies keeping track of them.
     PCPMappingNonce pcp_nonce;
@@ -108,7 +106,7 @@ static bool ProcessPCP()
         // Sanity-check returned lifetime.
         if (actual_lifetime < 30) {
             LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "portmap: Got impossibly short mapping lifetime of %d seconds\n", actual_lifetime);
-            return false;
+            return;
         }
         // RFC6887 11.2.1 recommends that clients send their first renewal packet at a time chosen with uniform random
         // distribution in the range 1/2 to 5/8 of expiration time.
@@ -119,28 +117,13 @@ static bool ProcessPCP()
 
     // We don't delete the mappings when the thread is interrupted because this would add additional complexity, so
     // we rather just choose a fairly short expiry time.
-
-    return ret;
 }
 
 static void ThreadMapPort()
 {
-    bool ok;
     do {
-        ok = false;
-
-        if (g_mapport_enabled_protos & MapPortProtoFlag::PCP) {
-            g_mapport_current_proto = MapPortProtoFlag::PCP;
-            ok = ProcessPCP();
-            if (ok) continue;
-        }
-
-        g_mapport_current_proto = MapPortProtoFlag::NONE;
-        if (g_mapport_enabled_protos == MapPortProtoFlag::NONE) {
-            return;
-        }
-
-    } while (ok || g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD));
+        ProcessPCP();
+    } while (g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD));
 }
 
 void StartThreadMapPort()
@@ -151,46 +134,18 @@ void StartThreadMapPort()
     }
 }
 
-static void DispatchMapPort()
+void StartMapPort(bool enable)
 {
-    if (g_mapport_current_proto == MapPortProtoFlag::NONE && g_mapport_enabled_protos == MapPortProtoFlag::NONE) {
-        return;
-    }
-
-    if (g_mapport_current_proto == MapPortProtoFlag::NONE && g_mapport_enabled_protos != MapPortProtoFlag::NONE) {
+    if (enable) {
         StartThreadMapPort();
-        return;
-    }
-
-    if (g_mapport_current_proto != MapPortProtoFlag::NONE && g_mapport_enabled_protos == MapPortProtoFlag::NONE) {
+    } else {
         InterruptMapPort();
         StopMapPort();
-        return;
-    }
-
-    if (g_mapport_enabled_protos & g_mapport_current_proto) {
-        return;
     }
 }
 
-static void MapPortProtoSetEnabled(MapPortProtoFlag proto, bool enabled)
-{
-    if (enabled) {
-        g_mapport_enabled_protos |= proto;
-    } else {
-        g_mapport_enabled_protos &= ~proto;
-    }
-}
-
-void StartMapPort(bool use_pcp)
-{
-    MapPortProtoSetEnabled(MapPortProtoFlag::PCP, use_pcp);
-    DispatchMapPort();
-}
-
 void InterruptMapPort()
 {
-    g_mapport_enabled_protos = MapPortProtoFlag::NONE;
     if (g_mapport_thread.joinable()) {
         g_mapport_interrupt();
     }

src/mapport.h

@@ -7,13 +7,7 @@
 
 static constexpr bool DEFAULT_NATPMP = false;
 
-enum MapPortProtoFlag : unsigned int {
-    NONE = 0x00,
-    // 0x01 was for UPnP, for which we dropped support.
-    PCP = 0x02,   // PCP with NAT-PMP fallback.
-};
-
-void StartMapPort(bool use_pcp);
+void StartMapPort(bool enable);
 void InterruptMapPort();
 void StopMapPort();
 

src/node/interfaces.cpp

@@ -188,7 +188,7 @@ class NodeImpl : public Node
         });
         args().WriteSettingsFile();
     }
-    void mapPort(bool use_pcp) override { StartMapPort(use_pcp); }
+    void mapPort(bool enable) override { StartMapPort(enable); }
     bool getProxy(Network net, Proxy& proxy_info) override { return GetProxy(net, proxy_info); }
     size_t getNodeCount(ConnectionDirection flags) override
     {