Cleanups to port mapping module post UPnP drop

darosior commented at 7:21 pm on October 25, 2024: member

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

DrahtBot commented at 7:21 pm on October 25, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31157.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

darosior renamed this:
~~Cleanups to port mapping module post UPnP dropt~~
Cleanups to port mapping module post UPnP drop
on Oct 25, 2024

laanwj commented at 9:25 am on October 27, 2024: member

i’ll review this when #31130 is merged, i have some of my own ideas about portmap.cpp as well that i haven’t fully worked out yet.

laanwj requested review from laanwj on Oct 27, 2024

fanquake added this to the milestone 29.0 on Oct 28, 2024

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.

c4e82b854c

darosior force-pushed on Oct 28, 2024

darosior commented at 10:09 pm on October 28, 2024: member

Rebased after #31130 got merged.

darosior marked this as ready for review on Oct 28, 2024

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.

2a6536ceda

mapport: drop unnecessary function 9bd936fa34

in src/qt/optionsmodel.cpp:529 in f6855a369c outdated

525@@ -526,7 +526,7 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value, const std::
526     case MapPortNatpmp: // core option - can be changed on-the-fly
527         if (changed()) {
528             update(value.toBool());
529-            node().mapPort(value.toBool());
530+            if (value.toBool()) node().mapPort();

laanwj commented at 10:39 am on October 29, 2024:

By adding this if here, this no longer accounts for the situation where port mapping is turned from on to off. i think i slightly prefer the old API for mapPort.

darosior commented at 2:33 pm on October 29, 2024:

Ah, yeah, right, that was stupid. I’ll stick to just renaming use_pcp to enabled.

darosior commented at 4:55 pm on October 29, 2024:

Done.

darosior force-pushed on Oct 29, 2024

in src/mapport.cpp:154 in 9bd936fa34 outdated

152@@ -153,44 +153,23 @@ void StartThreadMapPort()
153 
154 static void DispatchMapPort()

laanwj commented at 10:18 am on October 30, 2024:

DispatchMapPort is local and only called in one place; might as well roll it into StartMapPort?

darosior commented at 10:48 pm on October 31, 2024:

Done.

mapport: merge DispatchMapPort into StartMapPort 1b223cb19b

mapport: remove unnecessary 'g_mapport_current' variable 8fb45fcda0

in src/mapport.cpp:156 in 9bd936fa34 outdated

152@@ -153,44 +153,23 @@ void StartThreadMapPort()
153 
154 static void DispatchMapPort()
155 {
156-    if (g_mapport_current_proto == MapPortProtoFlag::NONE && g_mapport_enabled_protos == MapPortProtoFlag::NONE) {
157-        return;
158-    }
159-
160-    if (g_mapport_current_proto == MapPortProtoFlag::NONE && g_mapport_enabled_protos != MapPortProtoFlag::NONE) {
161+    if (!g_mapport_current && g_mapport_enabled) {

laanwj commented at 10:23 am on October 30, 2024:

Maybe i’m missing something but the intent of g_mapport_current is hazy to me. It’s turned on in ThreadMapPort while ProcessPCP is running, then turned off temporarily when ProcessPCP returns false.

If we want to know if the mapport thread is running, g_mapport_thread.joinable() seems to suffice.
If we want to know if mapport thread is supposed to be running, or stop it, there’s g_mapport_enabled.

darosior commented at 9:46 pm on October 31, 2024:

Yes, as far as i can tell it’s an artifact of switching between protocols. I should remove it. In fact i think g_mapport_enabled can be removed as well as we can just use g_mapport_interrupt to stop it.

darosior commented at 10:49 pm on October 31, 2024:

Done for both current and enabled.

laanwj commented at 11:17 am on November 1, 2024:

Nice! Good catch.

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.

9e6cba2988

mapport: make ProcessPCP void

Its return value is now unused. (It was also effectively unused before the previous commit, just in a roundabout way).

70398ae05b

in src/mapport.cpp:126 in 28b21b8eef outdated

137-        if (g_mapport_enabled_protos == MapPortProtoFlag::NONE) {
138-            return;
139-        }
140-
141-    } while (ok || g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD));
142+    while ((!g_mapport_interrupt && ProcessPCP()) || g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD)) {}

laanwj commented at 11:17 am on November 1, 2024:

The thread is not supposed to exit if ProcessPCP() fails. It should be more persistent: it should just wait a while and try again, maybe for the user to join a different network, swap their router, upgrade firmware, etc.

darosior commented at 1:17 pm on November 1, 2024:

Yes, this will not exit if ProcessPCP fails. (Unless it’s interrupted of course.)

darosior commented at 1:39 pm on November 1, 2024:

Maybe i should make this loop clearer? Like:

 0diff --git a/src/mapport.cpp b/src/mapport.cpp
 1index b303017144..8df78f435d 100644
 2--- a/src/mapport.cpp
 3+++ b/src/mapport.cpp
 4@@ -123,7 +123,9 @@ static bool ProcessPCP()
 5 
 6 static void ThreadMapPort()
 7 {
 8-    while ((!g_mapport_interrupt && ProcessPCP()) || g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD)) {}
 9+    do {
10+	ProcessPCP();
11+    } while (g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD));
12 }
13 
14 void StartThreadMapPort()

Which also makes clearer that ProcessPCP() should actually be void.

laanwj commented at 3:35 pm on November 1, 2024:

Yes, that seems better. Making ProcessPCP a void is also fine with me.

darosior commented at 7:37 pm on November 4, 2024:

Done.

darosior force-pushed on Nov 4, 2024

Cleanups to port mapping module post UPnP drop #31157

Code Coverage & Benchmarks

Reviews

Conflicts