From c4e82b854cdd3702d20a6afc49ef26bda9d2ba24 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 25 Oct 2024 15:07:24 -0400 Subject: [PATCH 1/7] mapport: make 'enabled' and 'current' bool Since there is only a single protocol now, clarify the code by changing the protocol enum for a bool for both variables. --- src/mapport.cpp | 40 ++++++++++++---------------------------- src/mapport.h | 6 ------ 2 files changed, 12 insertions(+), 34 deletions(-) diff --git a/src/mapport.cpp b/src/mapport.cpp index b7aadad6b41..96aa1020dde 100644 --- a/src/mapport.cpp +++ b/src/mapport.cpp @@ -25,8 +25,8 @@ static CThreadInterrupt g_mapport_interrupt; static std::thread g_mapport_thread; -static std::atomic_uint g_mapport_enabled_protos{MapPortProtoFlag::NONE}; -static std::atomic g_mapport_current_proto{MapPortProtoFlag::NONE}; +static std::atomic_bool g_mapport_enabled{false}; +static std::atomic_bool g_mapport_current{false}; using namespace std::chrono_literals; static constexpr auto PORT_MAPPING_REANNOUNCE_PERIOD{20min}; @@ -129,14 +129,14 @@ static void ThreadMapPort() do { ok = false; - if (g_mapport_enabled_protos & MapPortProtoFlag::PCP) { - g_mapport_current_proto = MapPortProtoFlag::PCP; + if (g_mapport_enabled) { + g_mapport_current = true; ok = ProcessPCP(); if (ok) continue; } - g_mapport_current_proto = MapPortProtoFlag::NONE; - if (g_mapport_enabled_protos == MapPortProtoFlag::NONE) { + g_mapport_current = false; + if (!g_mapport_enabled) { return; } @@ -153,44 +153,28 @@ void StartThreadMapPort() static void DispatchMapPort() { - 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 (!g_mapport_current && g_mapport_enabled) { StartThreadMapPort(); - return; - } - - if (g_mapport_current_proto != MapPortProtoFlag::NONE && g_mapport_enabled_protos == MapPortProtoFlag::NONE) { + } else if (g_mapport_current && !g_mapport_enabled) { InterruptMapPort(); StopMapPort(); - return; - } - - if (g_mapport_enabled_protos & g_mapport_current_proto) { - return; } } -static void MapPortProtoSetEnabled(MapPortProtoFlag proto, bool enabled) +static void MapPortProtoSetEnabled(bool enabled) { - if (enabled) { - g_mapport_enabled_protos |= proto; - } else { - g_mapport_enabled_protos &= ~proto; - } + g_mapport_enabled = enabled; } void StartMapPort(bool use_pcp) { - MapPortProtoSetEnabled(MapPortProtoFlag::PCP, use_pcp); + MapPortProtoSetEnabled(use_pcp); DispatchMapPort(); } void InterruptMapPort() { - g_mapport_enabled_protos = MapPortProtoFlag::NONE; + g_mapport_enabled = false; if (g_mapport_thread.joinable()) { g_mapport_interrupt(); } diff --git a/src/mapport.h b/src/mapport.h index 8e33ede32f6..aaf15051e71 100644 --- a/src/mapport.h +++ b/src/mapport.h @@ -7,12 +7,6 @@ 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 InterruptMapPort(); void StopMapPort(); From 2a6536ceda7a28c5605005032bba32ffd3df082d Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 29 Oct 2024 11:58:51 -0400 Subject: [PATCH 2/7] mapport: rename 'use_pcp' to 'enable' There is only a single protocol now, caller should just be concerned about whether to enable port mapping or not. --- src/interfaces/node.h | 2 +- src/mapport.cpp | 4 ++-- src/mapport.h | 2 +- src/node/interfaces.cpp | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/interfaces/node.h b/src/interfaces/node.h index aebb4386511..b7bcb431214 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -121,7 +121,7 @@ public: 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; diff --git a/src/mapport.cpp b/src/mapport.cpp index 96aa1020dde..066370287d8 100644 --- a/src/mapport.cpp +++ b/src/mapport.cpp @@ -166,9 +166,9 @@ static void MapPortProtoSetEnabled(bool enabled) g_mapport_enabled = enabled; } -void StartMapPort(bool use_pcp) +void StartMapPort(bool enable) { - MapPortProtoSetEnabled(use_pcp); + MapPortProtoSetEnabled(enable); DispatchMapPort(); } diff --git a/src/mapport.h b/src/mapport.h index aaf15051e71..3f16b2ea619 100644 --- a/src/mapport.h +++ b/src/mapport.h @@ -7,7 +7,7 @@ static constexpr bool DEFAULT_NATPMP = false; -void StartMapPort(bool use_pcp); +void StartMapPort(bool enable); void InterruptMapPort(); void StopMapPort(); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index e4ae9400e37..20c644b3479 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -187,7 +187,7 @@ public: }); 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 { From 9bd936fa34a2d8a1b0f71836456d7933fbf131a6 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 29 Oct 2024 11:59:36 -0400 Subject: [PATCH 3/7] mapport: drop unnecessary function --- src/mapport.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/mapport.cpp b/src/mapport.cpp index 066370287d8..a0a1b0a23aa 100644 --- a/src/mapport.cpp +++ b/src/mapport.cpp @@ -161,14 +161,9 @@ static void DispatchMapPort() } } -static void MapPortProtoSetEnabled(bool enabled) -{ - g_mapport_enabled = enabled; -} - void StartMapPort(bool enable) { - MapPortProtoSetEnabled(enable); + g_mapport_enabled = enable; DispatchMapPort(); } From 1b223cb19b4e04f9ab5fa0750e804fe44eba012f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 31 Oct 2024 16:37:33 -0400 Subject: [PATCH 4/7] mapport: merge DispatchMapPort into StartMapPort --- src/mapport.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/mapport.cpp b/src/mapport.cpp index a0a1b0a23aa..833b52a2088 100644 --- a/src/mapport.cpp +++ b/src/mapport.cpp @@ -151,8 +151,9 @@ void StartThreadMapPort() } } -static void DispatchMapPort() +void StartMapPort(bool enable) { + g_mapport_enabled = enable; if (!g_mapport_current && g_mapport_enabled) { StartThreadMapPort(); } else if (g_mapport_current && !g_mapport_enabled) { @@ -161,12 +162,6 @@ static void DispatchMapPort() } } -void StartMapPort(bool enable) -{ - g_mapport_enabled = enable; - DispatchMapPort(); -} - void InterruptMapPort() { g_mapport_enabled = false; From 8fb45fcda07aa1fa6d8420cbfe8e6b960b2df8fb Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 31 Oct 2024 16:42:47 -0400 Subject: [PATCH 5/7] mapport: remove unnecessary 'g_mapport_current' variable --- src/mapport.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/mapport.cpp b/src/mapport.cpp index 833b52a2088..2bb33cdbd92 100644 --- a/src/mapport.cpp +++ b/src/mapport.cpp @@ -26,7 +26,6 @@ static CThreadInterrupt g_mapport_interrupt; static std::thread g_mapport_thread; static std::atomic_bool g_mapport_enabled{false}; -static std::atomic_bool g_mapport_current{false}; using namespace std::chrono_literals; static constexpr auto PORT_MAPPING_REANNOUNCE_PERIOD{20min}; @@ -130,12 +129,10 @@ static void ThreadMapPort() ok = false; if (g_mapport_enabled) { - g_mapport_current = true; ok = ProcessPCP(); if (ok) continue; } - g_mapport_current = false; if (!g_mapport_enabled) { return; } @@ -154,9 +151,9 @@ void StartThreadMapPort() void StartMapPort(bool enable) { g_mapport_enabled = enable; - if (!g_mapport_current && g_mapport_enabled) { + if (g_mapport_enabled) { StartThreadMapPort(); - } else if (g_mapport_current && !g_mapport_enabled) { + } else { InterruptMapPort(); StopMapPort(); } From 9e6cba29882e72eabbe68307b2040d7919143b6f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 31 Oct 2024 18:43:31 -0400 Subject: [PATCH 6/7] mapport: remove unnecessary 'g_mapport_enabled' It was only necessary for switching between mapping protocols. It's also used to return in ThreadMapPort but we can just use the interrupt for this purpose. --- src/mapport.cpp | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/mapport.cpp b/src/mapport.cpp index 2bb33cdbd92..ba45219f78b 100644 --- a/src/mapport.cpp +++ b/src/mapport.cpp @@ -25,7 +25,6 @@ static CThreadInterrupt g_mapport_interrupt; static std::thread g_mapport_thread; -static std::atomic_bool g_mapport_enabled{false}; using namespace std::chrono_literals; static constexpr auto PORT_MAPPING_REANNOUNCE_PERIOD{20min}; @@ -124,20 +123,9 @@ static bool ProcessPCP() static void ThreadMapPort() { - bool ok; do { - ok = false; - - if (g_mapport_enabled) { - ok = ProcessPCP(); - if (ok) continue; - } - - if (!g_mapport_enabled) { - 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() @@ -150,8 +138,7 @@ void StartThreadMapPort() void StartMapPort(bool enable) { - g_mapport_enabled = enable; - if (g_mapport_enabled) { + if (enable) { StartThreadMapPort(); } else { InterruptMapPort(); @@ -161,7 +148,6 @@ void StartMapPort(bool enable) void InterruptMapPort() { - g_mapport_enabled = false; if (g_mapport_thread.joinable()) { g_mapport_interrupt(); } From 70398ae05bc36a2788e87f67ae06962f43fe35a6 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 4 Nov 2024 14:19:40 -0500 Subject: [PATCH 7/7] mapport: make ProcessPCP void Its return value is now unused. (It was also effectively unused before the previous commit, just in a roundabout way). --- src/mapport.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/mapport.cpp b/src/mapport.cpp index ba45219f78b..83105f51fdc 100644 --- a/src/mapport.cpp +++ b/src/mapport.cpp @@ -30,7 +30,7 @@ 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; @@ -106,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. @@ -117,8 +117,6 @@ 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()