Cleanups to port mapping module post UPnP drop #31157

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.

Type	Reviewers
ACK	laanwj, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

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:

diff --git a/src/mapport.cpp b/src/mapport.cpp
index b303017144..8df78f435d 100644
--- a/src/mapport.cpp
+++ b/src/mapport.cpp
@@ -123,7 +123,9 @@ static bool ProcessPCP()
 
 static void ThreadMapPort()
 {
-    while ((!g_mapport_interrupt && ProcessPCP()) || g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD)) {}
+    do {
+	ProcessPCP();
+    } while (g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD));
 }
 
 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

darosior commented at 3:33 AM on January 14, 2025: member

I was playing with a couple routers and figured i'd test this. Could successfully map ports against an OPNSense box and a Spectrum "WIFI 6E router".

laanwj added the label P2P on Jan 14, 2025

laanwj approved

laanwj commented at 9:12 AM on January 15, 2025: member

Code review ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6

glozow requested review from TheCharlatan on Feb 13, 2025

glozow requested review from achow101 on Feb 13, 2025

in src/mapport.cpp:119 in 70398ae05b

 116 | @@ -119,28 +117,13 @@ static bool ProcessPCP()
 117 |  
 118 |      // We don't delete the mappings when the thread is interrupted because this would add additional complexity, so
 119 |      // we rather just choose a fairly short expiry time.

TheCharlatan commented at 7:18 PM on February 13, 2025:

This comment seems to be lacking context now.

darosior commented at 8:02 PM on February 13, 2025:

Why? If we are interrupted the while() clause above will continue here and we won't tell the router to remove the mappings and just return instead.

TheCharlatan commented at 9:16 AM on February 14, 2025:

Ah, I was irritated by the reference to the expiry, but it is only a couple of lines above, so I guess that is fine as is.

TheCharlatan approved

TheCharlatan commented at 9:38 AM on February 14, 2025: contributor

ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6

Just a nit: Maybe cleanup the dangling includes?

fanquake merged this on Feb 14, 2025

fanquake closed this on Feb 14, 2025

darosior deleted the branch on Feb 18, 2025