daemon: remove UPnP support

Keep the "-upnp" option as a hidden arg for one major version in order
to show a more user friendly error to people who had this option set in
their config file.

Author: darosior

src/init.cpp

@@ -561,11 +561,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     argsman.AddArg("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port of %i will be used.", DEFAULT_TOR_CONTROL, DEFAULT_TOR_CONTROL_PORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION);
-#ifdef USE_UPNP
-    argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %u)", DEFAULT_UPNP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-#else
-    hidden_args.emplace_back("-upnp");
-#endif
+    // UPnP support was dropped. We keep `-upnp` as a hidden arg to display a more user friendly error when set. TODO: remove (here and below) for 30.0.
+    argsman.AddArg("-upnp", "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN);
     argsman.AddArg("-natpmp", strprintf("Use PCP or NAT-PMP to map the listening port (default: %u)", DEFAULT_NATPMP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-whitebind=<[permissions@]addr>", "Bind to the given address and add permission flags to the peers connecting to it. "
         "Use [host]:port notation for IPv6. Allowed permissions: " + Join(NET_PERMISSIONS_DOC, ", ") + ". "
@@ -740,8 +737,6 @@ void InitParameterInteraction(ArgsManager& args)
             LogInfo("parameter interaction: -proxy set -> setting -listen=0\n");
         // to protect privacy, do not map ports when a proxy is set. The user may still specify -listen=1
         // to listen locally, so don't rely on this happening through -listen below.
-        if (args.SoftSetBoolArg("-upnp", false))
-            LogInfo("parameter interaction: -proxy set -> setting -upnp=0\n");
         if (args.SoftSetBoolArg("-natpmp", false)) {
             LogInfo("parameter interaction: -proxy set -> setting -natpmp=0\n");
         }
@@ -752,8 +747,6 @@ void InitParameterInteraction(ArgsManager& args)
 
     if (!args.GetBoolArg("-listen", DEFAULT_LISTEN)) {
         // do not map ports or try to retrieve public IP when not listening (pointless)
-        if (args.SoftSetBoolArg("-upnp", false))
-            LogInfo("parameter interaction: -listen=0 -> setting -upnp=0\n");
         if (args.SoftSetBoolArg("-natpmp", false)) {
             LogInfo("parameter interaction: -listen=0 -> setting -natpmp=0\n");
         }
@@ -877,6 +870,12 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 
     // also see: InitParameterInteraction()
 
+    // We drop UPnP support but kept the arg as hidden for now to display a friendlier error to user who have the
+    // option in their config. TODO: remove (here and above) for version 30.0.
+    if (args.IsArgSet("-upnp")) {
+        return InitError(_("UPnP support was dropped in version 29.0. Consider using '-natpmp' instead."));
+    }
+
     // Error if network-specific options (-addnode, -connect, etc) are
     // specified in default section of config file, but not overridden
     // on the command line or in this chain's section of the config file.
@@ -1827,8 +1826,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     LogPrintf("nBestHeight = %d\n", chain_active_height);
     if (node.peerman) node.peerman->SetBestBlock(chain_active_height, std::chrono::seconds{best_block_time});
 
-    // Map ports with UPnP or NAT-PMP
-    StartMapPort(args.GetBoolArg("-upnp", DEFAULT_UPNP), args.GetBoolArg("-natpmp", DEFAULT_NATPMP));
+    // Map ports with NAT-PMP
+    StartMapPort(false, args.GetBoolArg("-natpmp", DEFAULT_NATPMP));
 
     CConnman::Options connOptions;
     connOptions.m_local_services = g_local_services;

src/mapport.cpp

@@ -2,8 +2,6 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <bitcoin-build-config.h> // IWYU pragma: keep
-
 #include <mapport.h>
 
 #include <clientversion.h>
@@ -18,15 +16,6 @@
 #include <util/thread.h>
 #include <util/threadinterrupt.h>
 
-#ifdef USE_UPNP
-#include <miniupnpc/miniupnpc.h>
-#include <miniupnpc/upnpcommands.h>
-#include <miniupnpc/upnperrors.h>
-// The minimum supported miniUPnPc API version is set to 17. This excludes
-// versions with known vulnerabilities.
-static_assert(MINIUPNPC_API_VERSION >= 17, "miniUPnPc API version >= 17 assumed");
-#endif // USE_UPNP
-
 #include <atomic>
 #include <cassert>
 #include <chrono>
@@ -134,78 +123,6 @@ static bool ProcessPCP()
     return ret;
 }
 
-#ifdef USE_UPNP
-static bool ProcessUpnp()
-{
-    bool ret = false;
-    std::string port = strprintf("%u", GetListenPort());
-    const char * multicastif = nullptr;
-    const char * minissdpdpath = nullptr;
-    struct UPNPDev * devlist = nullptr;
-    char lanaddr[64];
-
-    int error = 0;
-    devlist = upnpDiscover(2000, multicastif, minissdpdpath, 0, 0, 2, &error);
-
-    struct UPNPUrls urls;
-    struct IGDdatas data;
-    int r;
-#if MINIUPNPC_API_VERSION <= 17
-    r = UPNP_GetValidIGD(devlist, &urls, &data, lanaddr, sizeof(lanaddr));
-#else
-    r = UPNP_GetValidIGD(devlist, &urls, &data, lanaddr, sizeof(lanaddr), nullptr, 0);
-#endif
-    if (r == 1)
-    {
-        if (fDiscover) {
-            char externalIPAddress[40];
-            r = UPNP_GetExternalIPAddress(urls.controlURL, data.first.servicetype, externalIPAddress);
-            if (r != UPNPCOMMAND_SUCCESS) {
-                LogPrintf("UPnP: GetExternalIPAddress() returned %d\n", r);
-            } else {
-                if (externalIPAddress[0]) {
-                    std::optional<CNetAddr> resolved{LookupHost(externalIPAddress, false)};
-                    if (resolved.has_value()) {
-                        LogPrintf("UPnP: ExternalIPAddress = %s\n", resolved->ToStringAddr());
-                        AddLocal(resolved.value(), LOCAL_MAPPED);
-                    }
-                } else {
-                    LogPrintf("UPnP: GetExternalIPAddress failed.\n");
-                }
-            }
-        }
-
-        std::string strDesc = PACKAGE_NAME " " + FormatFullVersion();
-
-        do {
-            r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, port.c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", nullptr, "0");
-
-            if (r != UPNPCOMMAND_SUCCESS) {
-                ret = false;
-                LogPrintf("AddPortMapping(%s, %s, %s) failed with code %d (%s)\n", port, port, lanaddr, r, strupnperror(r));
-                break;
-            } else {
-                ret = true;
-                LogPrintf("UPnP Port Mapping successful.\n");
-            }
-        } while (g_mapport_interrupt.sleep_for(PORT_MAPPING_REANNOUNCE_PERIOD));
-        g_mapport_interrupt.reset();
-
-        r = UPNP_DeletePortMapping(urls.controlURL, data.first.servicetype, port.c_str(), "TCP", nullptr);
-        LogPrintf("UPNP_DeletePortMapping() returned: %d\n", r);
-        freeUPNPDevlist(devlist); devlist = nullptr;
-        FreeUPNPUrls(&urls);
-    } else {
-        LogPrintf("No valid UPnP IGDs found\n");
-        freeUPNPDevlist(devlist); devlist = nullptr;
-        if (r != 0)
-            FreeUPNPUrls(&urls);
-    }
-
-    return ret;
-}
-#endif // USE_UPNP
-
 static void ThreadMapPort()
 {
     bool ok;
@@ -219,15 +136,6 @@ static void ThreadMapPort()
             if (ok) continue;
         }
 
-#ifdef USE_UPNP
-        // Low priority protocol.
-        if (g_mapport_enabled_protos & MapPortProtoFlag::UPNP) {
-            g_mapport_current_proto = MapPortProtoFlag::UPNP;
-            ok = ProcessUpnp();
-            if (ok) continue;
-        }
-#endif // USE_UPNP
-
         g_mapport_current_proto = MapPortProtoFlag::NONE;
         if (g_mapport_enabled_protos == MapPortProtoFlag::NONE) {
             return;
@@ -268,7 +176,7 @@ static void DispatchMapPort()
 
     assert(g_mapport_thread.joinable());
     assert(!g_mapport_interrupt);
-    // Interrupt a protocol-specific loop in the ThreadUpnp() or in the ThreadPCP()
+    // Interrupt a protocol-specific loop in the ThreadPCP()
     // to force trying the next protocol in the ThreadMapPort() loop.
     g_mapport_interrupt();
 }
@@ -284,7 +192,6 @@ static void MapPortProtoSetEnabled(MapPortProtoFlag proto, bool enabled)
 
 void StartMapPort(bool use_upnp, bool use_pcp)
 {
-    MapPortProtoSetEnabled(MapPortProtoFlag::UPNP, use_upnp);
     MapPortProtoSetEnabled(MapPortProtoFlag::PCP, use_pcp);
     DispatchMapPort();
 }

src/mapport.h

@@ -5,13 +5,11 @@
 #ifndef BITCOIN_MAPPORT_H
 #define BITCOIN_MAPPORT_H
 
-static constexpr bool DEFAULT_UPNP = false;
-
 static constexpr bool DEFAULT_NATPMP = false;
 
 enum MapPortProtoFlag : unsigned int {
     NONE = 0x00,
-    UPNP = 0x01,
+    // 0x01 was for UPnP, for which we dropped support.
     PCP = 0x02,   // PCP with NAT-PMP fallback.
 };
 

src/net.h

@@ -148,7 +148,7 @@ enum
     LOCAL_NONE,   // unknown
     LOCAL_IF,     // address a local interface listens on
     LOCAL_BIND,   // address explicit bound to
-    LOCAL_MAPPED, // address reported by UPnP or PCP
+    LOCAL_MAPPED, // address reported by PCP
     LOCAL_MANUAL, // address explicitly specified (-externalip=)
 
     LOCAL_MAX

test/functional/test_framework/util.py

@@ -433,7 +433,6 @@ def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=
         # in tests.
         f.write("peertimeout=999999999\n")
         f.write("printtoconsole=0\n")
-        f.write("upnp=0\n")
         f.write("natpmp=0\n")
         f.write("shrinkdebugfile=0\n")
         f.write("deprecatedrpc=create_bdb\n")  # Required to run the tests