Close #11902 This PR is an alternative to:
To compile with NAT-PMP support on Ubuntu libnatpmp-dev should be available.
Log excerpt:
02020-02-05T20:12:28Z [mapport] NAT-PMP: public address = 95.164.65.194
12020-02-05T20:12:28Z [mapport] AddLocal(95.164.65.194:18333,3)
22020-02-05T20:12:28Z [mapport] NAT-PMP: port mapping successful.
See: libnatpmp
Some follow-ups are out of this PR’s scope:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
114+ }
115+
116+ closenatpmp(&natpmp);
117+}
118+
119+#else // USE_UPNP
This will break building with both NAT-PMP and UPnP support…
Yes, I’m aware of that. This PR in its current state is intended for NAT-PMP functionality tests and concept (N)ACKs.
Updated 4ff5349a3172d02fceb976bf4989996c88f86680 -> cf94431e8f4bfd6636998a17e2355feb2c5c3d58 (pr18077.03 -> pr18077.04, compare):
added libnatpmp package to macOS 10.14 native Travis build.
Updated cf94431e8f4bfd6636998a17e2355feb2c5c3d58 -> 88248a44241c8e3922a80729ce4a9a178ef2dbea (pr18077.04 -> pr18077.05, compare):
added:
Concept ACK. Lightly tested configure / build on macOS.
Unfortunately I’m behind a IPv6 DS-Lite router, I can’t test the IPv4 port forward. Perhaps -7 could be replaced by something more readable.
02020-02-11T17:48:27Z natpmp thread start
12020-02-11T17:48:27Z Bound to [::]:8333
22020-02-11T17:48:27Z Bound to 0.0.0.0:8333
32020-02-11T17:48:27Z init message: Loading P2P addresses...
42020-02-11T17:48:27Z NAT-PMP: readnatpmpresponseorretry() for public address failed with -7 error.
52020-02-11T17:48:27Z NAT-PMP: readnatpmpresponseorretry() for port mapping failed with -7 error.
I looked up the error code, and it’s what I would expect given my setup:
0/* NATPMP_ERR_NOGATEWAYSUPPORT : the gateway does not support NAT-PMP */
1#define NATPMP_ERR_NOGATEWAYSUPPORT (-7)
Two suggesitons for followups, or this PR:
Thank you for your review and testing.
I looked up the error code, and it’s what I would expect given my setup:
0/* NATPMP_ERR_NOGATEWAYSUPPORT : the gateway does not support NAT-PMP */ 1#define NATPMP_ERR_NOGATEWAYSUPPORT (-7)
Agree.
But in this initial PR I intentionally do not provide user-friendly error messages in order to keep ThreadNatpmp() dense and tight for easier reviewing ;)
In followups every library call (initnatpmp(), sendpublicaddressrequest() and sendnewportmappingrequest()) with the following readnatpmpresponseorretry() loops could be refactored out to separate functions with user-friendly error messages.
Two suggesitons for followups, or this PR:
* add checkbox in QT Settings (above UPNP)
Agree. From the OP:
Some follow-ups are out of this PR’s scope:
* mention NAT-PMP library in the version message * integrate NAT-PMP into the GUI
194+#endif // #ifdef USE_NATPMP
195+ return;
196+ }
197+ case MapPort::UPNP: {
198+#ifdef USE_UPNP
199+ g_mapport_thread = std::thread((std::bind(&TraceThread<void (*)()>, "upnp", &ThreadUpnp)));
g_mapport_thread set above if both are enabled?
It should be possible to enable both.
Do you mean that a router could do port mapping using both NAT-PMP and UPnP simultaneously? What is the reason for that?
I wouldn’t use them simultaneously, but one fails it makes sense to try the other.
Good suggestion for a followup.
Though I believe our long plan is to remove UPnP, if NAT-PMP does the job.
Not all routers with stock software have NAT-PMP port mapping feature.
Users who don’t know what their router supports should be able to just flip both on. And laptops might very well migrate between different routers using each protocol.
Agree with @hebasto that NAT-PMP can’t be a complete substitute for UPnP since routers might not support both.
Updated c7e90ca40ad9b7c66f3429b792435e9279ceca6b -> dc228d6db8d2e9406d6d6e0eb04e37940b1ffb82 (pr18077.06 -> pr18077.07, compare):
Changes:
NATPMP_ERR_NOGATEWAYSUPPORT error-natpmp and -upnp options are suppliedPerhaps
-7could be replaced by something more readable.
Done.
If both are enabled, would you try to open de port with NAT_PMP first and if that fails UPNP? I wouldn’t use them simultaneously, but one fails it makes sense to try the other.
Done.
451 #ifdef USE_UPNP
452 #if USE_UPNP
453 gArgs.AddArg("-upnp", "Use UPnP to map the listening port (default: 1 when listening and no -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
454 #else
455- gArgs.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %u)", 0), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
456+ gArgs.AddArg("-upnp", "Use UPnP to map the listening port (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
%s in the new options)
The logic here still seems broken. Again, consider that users may migrate between different routers each with NAT-PMP or UPnP support. So there should be a common thread main that at each refresh (which ideally should trigger when network availability changes, but that can be another PR) checks for which protocol to use and does the right thing.
ThreadAnyAvailable() does almost the same:
if a used protocol becomes unavailable, ThreadAnyAvailable() switches to another one, no?
Updated dc228d6db8d2e9406d6d6e0eb04e37940b1ffb82 -> 2d5d98ce0aa07401c6771731f4e27634629f197c (pr18077.07 -> pr18077.08, compare):
Changes:
1784+ // Map ports with NAT-PMP (preferable) or UPnP.
1785+ {
1786+ const bool natpmp = gArgs.GetBoolArg("-natpmp", DEFAULT_NATPMP);
1787+ const bool upnp = gArgs.GetBoolArg("-upnp", DEFAULT_UPNP);
1788+ if (natpmp && upnp) {
1789+ StartMapPort(MapPort::ANY_AVAILABLE);
StartMapPort(bool use_upnp, bool use_natpmp)
361@@ -350,9 +362,13 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
362 fMinimizeToTray = value.toBool();
363 settings.setValue("fMinimizeToTray", fMinimizeToTray);
364 break;
365+ case MapPortNatpmp: // core option - can be changed on-the-fly
366+ settings.setValue("fUseNatpmp", value.toBool());
367+ m_node.mapPort(value.toBool(), MapPort::NAT_PMP);
438@@ -438,6 +439,15 @@ void SetupServerArgs()
439 gArgs.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
440 gArgs.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION);
441 gArgs.AddArg("-asmap=<file>", "Specify asn mapping used for bucketing of the peers. Path should be relative to the -datadir path.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
442+#ifdef USE_NATPMP
443+#if USE_NATPMP
444+ gArgs.AddArg("-natpmp", "Use NAT-PMP to map the listening port (default: 1 when listening and no -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
445+#else
446+ gArgs.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %u)", 0), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
%s > %u
%s>%u
What is the rationale? We already use the %u specifier for unsigned integers in tfm::format().
%s doesn’t care what type the value is.
You can use the same format string above, with a value of "1 when listening and no -proxy"
Updated 2d5d98ce0aa07401c6771731f4e27634629f197c -> ccb4a7ea18c38914f084b7c070303d020adad326 (pr18077.08 -> pr18077.09, compare):
Hi all, did some testing on ccb4a7ea18c38914f084b7c070303d020adad326. What I did:
natpmpd, added relevant pf.conf line, reloaded pfbitcoind with --without-miniupnpc --disable-bench --disable-wallet --without-gui --with-natpmpbitcoind with -natpmp02020-02-18T22:26:26Z NAT-PMP: Public address = 108.21.84.253
12020-02-18T22:26:26Z AddLocal(108.21.84.253:8333,3)
22020-02-18T22:26:26Z NAT-PMP: Port mapping failed.
32020-02-18T22:26:26Z NAT-PMP: Port mapping removed successfully.
natpmpd anchored pfctl rules:0$ doas pfctl -a natpmpd -s rules
1pass in quick on rdomain 0 inet proto tcp from any to <my-egress-ip> port = 62774 flags any rdr-to 192.168.0.5 port 8333
I believe this means that natpmpd successfully allocated a port 62774 to bitcoind, but since that port does not equal our private port, this check failed: https://github.com/bitcoin/bitcoin/pull/18077/files#diff-161ad6a962291e0bbafeb83ddc8a977cR109. I believe this is because natpmpd ignores our “Suggested External Port”, which is perfectly valid, and we should probably account for that possibility.
@dongcarl
Thank you for testing this PR with natpmpd on OpenBSD (I’ve tested with miniupnpd on OpenWrt).
I believe this is because
natpmpdignores our “Suggested External Port”, which is perfectly valid, and we should probably account for that possibility.
Correct. From natpmpd docs:
The external TCP or UDP port assigned to the client is always randomised rather than giving the first client the port it actually requested and then trying to work out what to do for additional clients that want the same external port.
(highlighted in italics by me).
OTOH, RFC 6886 states:
If the client would prefer to have a high-numbered “anonymous” external port assigned, then it should set the Suggested External Port to zero, which indicates to the gateway that it should allocate a high-numbered port of its choosing. If the client would prefer instead to have the mapped external port be the same as its local internal port if possible (e.g., a web server listening on port 80 that would ideally like to have external port 80), then it should set the Suggested External Port to the desired value. However, the gateway is not obliged to assign the port suggested, and may choose not to, either for policy reasons (e.g., port 80 is reserved and clients may not request it) or because that port has already been assigned to some other client.
(highlighted in italics by me). @luke-jr
If NAT-PMP allows ignoring the suggested port, then we probably should handle that by advertising the correct port for the external IP in addrman.
Sure, we should.
Updated ccb4a7ea18c38914f084b7c070303d020adad326 -> 03e2733be1772ef69caf34e84c7b400f64a8ea9f (pr18077.09 -> pr18077.10, compare):
363 fMinimizeToTray = value.toBool();
364 settings.setValue("fMinimizeToTray", fMinimizeToTray);
365 break;
366+ case MapPortNatpmp: // core option - can be changed on-the-fly
367+ settings.setValue("fUseNatpmp", value.toBool());
368+ m_node.mapPort(value.toBool(), settings.value("fUseUPnP").toBool());
This logic breaks the “command line overrides GUI options” stuff above (see addOverriddenOption)…
Not really sure what the best solution here is.
268+
269+ if (g_mapport_current_proto == MapPortProto::NONE) {
270+ if (g_mapport_target_proto == MapPortProto::NONE) {
271+ return;
272+ }
273+ } else if (g_mapport_target_proto & g_mapport_current_proto) {
Sure it works. The log excerpt for the mentioned case:
02020-02-20T23:26:30Z [mapport] UPNP_DeletePortMapping() returned: 0
12020-02-20T23:26:30Z [mapport] AddLocal(95.164.65.194:18333,3)
22020-02-20T23:26:30Z [mapport] NAT-PMP: Port mapping successful. External address = 95.164.65.194:18333
g_mapport_target_proto != MapPortProto::NONE
(g_mapport_target_proto & g_mapport_current_proto)
0void StartMapPort(bool use_natpmp, bool use_upnp)
1{
2 // Both protocols are enabled, so
3 // g_mapport_target_proto == MapPortProto::NAT_PMP | MapPortProto::UPNP
4 //
5 // UPnP is currently using, so
6 // g_mapport_current_proto == MapPortProto::UPNP
7 //
8 // A user disables UPnP then passed arguments are:
9 // use_natpmp == true, use_upnp = false
10
11 if (use_natpmp) {
12 // Effectively noop here as MapPortProto::NAT_PMP is set already.
13 g_mapport_target_proto |= MapPortProto::NAT_PMP;
14 } else {
15 g_mapport_target_proto &= ~MapPortProto::NAT_PMP;
16 }
17
18 if (use_upnp) {
19 g_mapport_target_proto |= MapPortProto::UPNP;
20 } else {
21 // Unset MapPortProto::UPNP bit.
22 g_mapport_target_proto &= ~MapPortProto::UPNP;
23 }
24
25 // At this point variables are set to the following values:
26 // g_mapport_target_proto == MapPortProto::NAT_PMP
27 // g_mapport_current_proto == MapPortProto::UPNP
28 // ...
29 if (g_mapport_current_proto == MapPortProto::NONE) {
30 if (g_mapport_target_proto == MapPortProto::NONE) {
31 return;
32 }
33 // ... and the next condition evaluates to false.
34 } else if (g_mapport_target_proto & g_mapport_current_proto) {
35 return;
36 }
37
38 // Making the changes...
39 ...
40}
And logs confirm this logic.
279+ StopMapPort();
280+ return;
281+ }
282+
283+ if (g_mapport_thread.joinable()) {
284+ g_mapport_interrupt();
132+ struct in_addr external_ipv4_addr;
133+ if (NatpmpInit(&natpmp) && NatpmpDiscover(&natpmp, external_ipv4_addr)) {
134+ bool external_ip_discovered = false;
135+ const uint16_t private_port = GetListenPort();
136+ do {
137+ ret = NatpmpMapping(&natpmp, external_ipv4_addr, private_port, external_ip_discovered);
g_mapport_external_port variable.
I suggest putting the variable renames in a separate commit for easier review.
Also, since NAT-PMP apparently maps random ports often, I think we should prefer UPnP…
Updated 03e2733be1772ef69caf34e84c7b400f64a8ea9f -> dd3229e7af8563c69d26955c4207f73ae1d3bc94 (pr18077.10 -> pr18077.11, compare):
Also, since NAT-PMP apparently maps random ports often, I think we should prefer UPnP…
Does this behavior of NAT-PMP gateways hurt the entire p2p network?
OTOH, NAT-PMP seems more secure than UPnP (for example, one could compare numbers of CVEs for each protocol).
Does this behavior of NAT-PMP gateways hurt the entire p2p network?
DNS seeds can’t provide port numbers, so using anything other than 8333 reduces the list of nodes we can advertise. Currently, mine (which is IIRC more restrictive than others) has 3090 eligible nodes, so it’s not a big deal, but it’d be nicer to have a wider selection available than a narrower selection.
OTOH, NAT-PMP seems more secure than UPnP (for example, one could compare numbers of CVEs for each protocol).
The CVEs AFAIK are for the implementations, not the protocol.
In any case, when we are using either, I think we’re reduced to the weakest no matter which we prefer (a hostile router could block NAT-PMP to force fallback to UPnP).
131+ natpmp_t natpmp;
132+ struct in_addr external_ipv4_addr;
133+ if (NatpmpInit(&natpmp) && NatpmpDiscover(&natpmp, external_ipv4_addr)) {
134+ bool external_ip_discovered = false;
135+ const uint16_t private_port = GetListenPort();
136+ g_mapport_interrupt.reset();
249+ if (g_mapport_target_proto == MapPortProto::NONE) {
250+ g_mapport_current_proto = MapPortProto::NONE;
251+ return;
252+ }
253+
254+ } while (ok || g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD));
If the protocol switch is initiated by a user, the g_mapport_interrupt() call will end the internal protocol-specific loop in the ThreadNatpmp() or in the ThreadUpnp(), ok == true and g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD) will not call at all.
If the protocol switch is initiated by a protocol failure, ok == false and g_mapport_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD) will behave as intended.
Setting g_mapport_target_proto variable to MapPortProto::NONE also causes shutting off port mapping entirely.
Re NAT-PMP preference: Also, with the port number changing every startup, it will be hard for peers to reconnect or meaningfully pass on addr messages for you…
I think cd7a0f689c5 should get split up. It’s too hard to follow what it’s doing.
Updated dd3229e7af8563c69d26955c4207f73ae1d3bc94 -> d9ecda8fa0ebce5b6da972691b19f4425164ae43 (pr18077.11 -> pr18077.12, compare):
134+ bool external_ip_discovered = false;
135+ const uint16_t private_port = GetListenPort();
136+ do {
137+ ret = NatpmpMapping(&natpmp, external_ipv4_addr, private_port, external_ip_discovered);
138+ } while (ret && g_mapport_interrupt.sleep_for(PORT_MAPPING_REANNOUNCE_PERIOD));
139+ g_mapport_interrupt.reset();
116@@ -115,14 +117,39 @@ static void ThreadMapPort()
117 } while (g_upnp_interrupt.sleep_for(PORT_MAPPING_RETRY_PERIOD));
118 }
119
120-void StartMapPort()
121+void StartThread()
This name is way too vague…
Yeah… I’m not good at choosing English names, and will appreciate your suggestion.
11@@ -12,7 +12,12 @@ static const bool DEFAULT_UPNP = USE_UPNP;
12 static const bool DEFAULT_UPNP = false;
13 #endif
14
15-void StartMapPort();
16+enum MapPortProto : unsigned int {
enum class
It will require to define bitwise operators which are used here: https://github.com/bitcoin/bitcoin/blob/d9ecda8fa0ebce5b6da972691b19f4425164ae43/src/mapport.cpp#L297-L304
Maybe in follow-up?
50@@ -50,6 +51,10 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
51 #ifndef USE_UPNP
52 ui->mapPortUpnp->setEnabled(false);
53 #endif
54+ connect(this, &QDialog::accepted, [this](){
55+ QSettings settings;
56+ model->node().mapPort(settings.value("fUseUPnP").toBool());
Are we sure the settings are changed before the signal? Or could it be in parallel and therefore a race?
Yes, we are. Please consider the following slot: https://github.com/bitcoin/bitcoin/blob/d9ecda8fa0ebce5b6da972691b19f4425164ae43/src/qt/optionsdialog.cpp#L284-L289
All changes are submitted to the model in the mapper->submit() call before the QDialog::accepted() signal is emitted when the dialog has been accepted by calling accept().
Then the following connection: https://github.com/bitcoin/bitcoin/blob/d9ecda8fa0ebce5b6da972691b19f4425164ae43/src/qt/optionsdialog.cpp#L57-L60 does its work.
In commit “gui: Apply port mapping changes on dialog exit” (58e8364dcdc4e57b0caac09f8402e6535301de9b)
@hebasto It seems like this commit has a bug and will incorrectly overwrite command line and config file -upnp and -natpmp options. I think this might have happened before, but previously only if these settings were changed, now if the dialog is just opened and closed.
Would you have an opinion on reverting this change? Aside from the bug, it seems simpler to go back to fully applying settings in OptionsModel, instead of using half-using OptionsModel to apply the setting, and then reading the setting later in a different code path and QSettings instance in dialog code outside of OptionsModel.
(Encountered this while rebasing #15936)
You are right.
It seems like this commit has a bug and will incorrectly overwrite command line and config file
-upnpand-natpmpoptions.
The concerns about user-friendly and correct behavior were raised in #18184. Now users are able to switch -upnp and -natpmp on/off on-the-fly via the GUI. Should we drop this feature?
I think this might have happened before, but previously only if these settings were changed, now if the dialog is just opened and closed.
Exiting the dialog via the “Cancel” push button or pressing the “Esc” key should not trigger that code path.
Anyway, I’ll happy to revert (or enhance) that change in a proper way.
446@@ -447,7 +447,12 @@ void SetupServerArgs()
447 #endif
448 #else
449 hidden_args.emplace_back("-upnp");
450-#endif
451+#endif // #ifdef USE_UPNP
452+#ifdef USE_NATPMP
453+ gArgs.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %s)", DEFAULT_NATPMP ? "1 when listening and no -proxy" : 0), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
a?b:c requires b and c to have the same type.
a?b:c requires b and c to have the same type.
Updated d9ecda8fa0ebce5b6da972691b19f4425164ae43 -> 405e85c61a6690ff2f7159c51715fdbafcbf2b1e (pr18077.12 -> pr18077.13, compare):
StartThread() renamed to StartThreadMapPort() (as @luke-jr suggested)MapPortProto enum to MapPortProtoFlag and g_mapport_target_proto to g_mapport_enabled_protos for better semantics0@@ -0,0 +1,137 @@
1+// Copyright (c) 2020 The Bitcoin Core developers
In commit “refactor: Move port mapping code to own files”
Nit: when you’re copying/moving code, retain the copyright years of the original code.
34@@ -35,9 +35,11 @@ static_assert(MINIUPNPC_API_VERSION >= 10, "miniUPnPc API version >= 10 assumed"
35 static CThreadInterrupt g_upnp_interrupt;
36 static std::thread g_upnp_thread;
37 static constexpr auto PORT_MAPPING_REANNOUNCE_PERIOD = std::chrono::minutes(20);
38+static constexpr auto PORT_MAPPING_RETRY_PERIOD = std::chrono::minutes(5);
39
40-static void ThreadMapPort()
41+static bool ThreadUpnp()
In commit “net: Keep trying to use UPnP when -upnp=1”
This is confusingly named when it isn’t actually a thread on its own anymore.
Updated 405e85c61a6690ff2f7159c51715fdbafcbf2b1e -> 4e7b9f03b8a3dd02bc9831e77905a8022770c846 (pr18077.13 -> pr18077.14, compare):
tACK f4a4643712eca819a5b7ed0a143216e06125d1a9 on Debian 9
02020-05-17T04:14:52Z NAT-PMP: Port mapping successful. External address = [redacted]:8333
12020-05-17T04:22:05Z NAT-PMP: Port mapping removed successfully.
The port is accessible externally without any other forwarding rules.
Rebased a660ecc26e691ed9b931d6fe27c26899ef66be64 -> d4a143ae233fc5fd0cb3c08ce7c388434d256574 (pr18077.28 -> pr18077.29, diff):
libnatpmp build with the custom CC variable.
For those looking to review or revisit:
(All pre-rebase) There are concept ACKs from: luke-jr, dongcarl, Sjors, Sipa a tACK from tryphe with no NACKs or blocking issues
Tested 225f512438f4391f6af255f851131358e3c466a2, works:
02020-08-20T19:07:08Z NAT-PMP: Port mapping successful. External address = <redacted>
Did not read code yet
0@@ -0,0 +1,9 @@
1+P2P and network changes
2+-----------------------
3+
4+- Added NAT-PMP port mapping support via [`libnatpmp`](https://miniupnp.tuxfamily.org/libnatpmp.html)
5+
6+Command-line options
7+--------------------
8+
9+- The `-natpmp` option has been added to use NAT-PMP to map the listening port. If both `-upnp` and `-natpmp` options are provided, the former prevails.
Perhaps I’m reading the code wrong, but it seems like “If both UPnP and NAT-PMP are enabled, a successful allocation from UPnP prevails over one from NAT-PMP” is a more accurate description here?
I’m thinking about the case where both UPnP and NAT-PMP are enabled, but the gateway box is not responding to UPnP, in that case, a NAT-PMP allocation would still be accepted, right?
Perhaps I’m reading the code wrong, but it seems like “If both UPnP and NAT-PMP are enabled, a successful allocation from UPnP prevails over one from NAT-PMP” is a more accurate description here?
Thanks! It remained from the initial version of the PR :) Updated.
I’m thinking about the case where both UPnP and NAT-PMP are enabled, but the gateway box is not responding to UPnP, in that case, a NAT-PMP allocation would still be accepted, right
The do loop introduced in the c01073db04e94a8d67f29de3daead0e2574b1c26 commit is responsible for that.
I performed basic functional testing against this branch (but not master with this applied due to rebase hell) against http://www.nongnu.org/natpmp/.
I don’t really like the natpmp default being set by configure. I see the UPNP is currently handled that way, but I think that is an artefact of the fact that UPNP is disabled for implementation specific security reasons. Otherwise, no other bitcoin setting has its default set by configure. That said, this could easily be something changed after getting the functionality merged or even in a subsequent release.
I found that it successfully mapped 8333 and released the mapping on shutdown.
I restricted the port range at the pmp-daemon and found that it successfully mapped a different port and correctly learned the other port number.
I killed the pmp-daemon while bitcoind was running then shutdown and verified that it didn’t hang up waiting for some unmap confirmation or anything like that.
I did not change my external IP to confirm that it learned about the IP change (as I couldn’t do that without disrupting other people). I think this would be a useful thing to test, but even if it didn’t work right I don’t think it would block merging this.
Looks to me like it works!
Rebased 44a697619d3fc3d9311c3d099cae04b21de071f3 -> 81a791d88d68e64968e52d8ff6ec20879bd5fad1 (pr18077.38 -> pr18077.39) due to the conflict with #19077, and addressed @gmaxwell’s comment:
ultra minor nit, ignore if you like: I didn’t like that the logs calls it “NAT-PMP” but the configuration option is natpmp, so I had to go read the code to figure out what I should be looking for. I’d prefer to see logs say “natpmp” for optimum greppability.
0@@ -0,0 +1,19 @@
1+package=libnatpmp
2+$(package)_version=20150609
3+$(package)_download_path=https://miniupnp.tuxfamily.org/files/
4+$(package)_file_name=$(package)-$($(package)_version).tar.gz
5+$(package)_sha256_hash=e1aa9c4c4219bc06943d6b2130f664daee213fb262fcb94dd355815b8f4536b0
Verified this locally.
0% curl -O https://miniupnp.tuxfamily.org/files/libnatpmp-20150609.tar.gz
1% sha256sum libnatpmp-20150609.tar.gz
2e1aa9c4c4219bc06943d6b2130f664daee213fb262fcb94dd355815b8f4536b0 libnatpmp-20150609.tar.gz
29+#include <chrono>
30+#include <functional>
31+#include <string>
32+#include <thread>
33+
34+#ifdef USE_UPNP
https://github.com/bitcoin/bitcoin/pull/18077/commits/035a41ca371675d0cb0fbfa8177553ec19068663
Verified this is move-only from here on down.
32@@ -33,11 +33,6 @@
33 #include <memory>
34 #include <condition_variable>
35
36-#ifndef WIN32
37-#include <arpa/inet.h>
https://github.com/bitcoin/bitcoin/pull/18077/commits/035a41ca371675d0cb0fbfa8177553ec19068663
So this was just unnecessary in the first place?
34@@ -35,9 +35,11 @@ static_assert(MINIUPNPC_API_VERSION >= 10, "miniUPnPc API version >= 10 assumed"
35 static CThreadInterrupt g_upnp_interrupt;
(Note on https://github.com/bitcoin/bitcoin/pull/18077/commits/5b6af780967c428ec38fa18ae68a193d2d036f57 as a whole)
I certainly don’t have qualms with this change but it seems orthogonal to NAT-PMP per se. Is it necessary for NAT-PMP somehow? Just curious.
134+ InterruptMapPort();
135+ StopMapPort();
136+ }
137+}
138+
139+static void MapPortProtoSetEnabled(MapPortProtoFlag proto, bool enabled)
https://github.com/bitcoin/bitcoin/pull/18077/commits/ea445ad4bc5b4d2904cbe234a627700a6d22d32e
Note: simultaneous use of either NAT-PMP or upnp enabled in a later commit.
32@@ -33,8 +33,8 @@ static_assert(MINIUPNPC_API_VERSION >= 10, "miniUPnPc API version >= 10 assumed"
33 #include <thread>
34
35 #ifdef USE_UPNP
36-static CThreadInterrupt g_upnp_interrupt;
37-static std::thread g_upnp_thread;
38+static CThreadInterrupt g_mapport_interrupt;
129+{
130+ bool ret = false;
131+ natpmp_t natpmp;
132+ struct in_addr external_ipv4_addr;
133+ if (NatpmpInit(&natpmp) && NatpmpDiscover(&natpmp, external_ipv4_addr)) {
134+ bool external_ip_discovered = false;
https://github.com/bitcoin/bitcoin/pull/18077/commits/f6bd717f3f44f2a167ce2e548df1132789d0ea98
Hm, I see we’re explicitly setting this in NetpmpMapping but then it goes unused. No big deal, just curious.
external_ip_discovered is used here https://github.com/bitcoin/bitcoin/blob/81a791d88d68e64968e52d8ff6ec20879bd5fad1/src/mapport.cpp#L104-L107 to prevent consecutive AddLocal calls.
478@@ -479,7 +479,12 @@ void SetupServerArgs(NodeContext& node)
479 #endif
480 #else
481 hidden_args.emplace_back("-upnp");
482-#endif
483+#endif // #ifdef USE_UPNP
484+#ifdef USE_NATPMP
485+ argsman.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %s)", DEFAULT_NATPMP ? "1 when listening and no -proxy" : "0"), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
test/lint/check-doc.py linter: https://travis-ci.org/github/bitcoin/bitcoin/jobs/738110074
49@@ -50,6 +50,9 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
50 /* Network elements init */
51 #ifndef USE_UPNP
52 ui->mapPortUpnp->setEnabled(false);
53+#endif
54+#ifndef USE_NATPMP
55+ ui->mapPortNatpmp->setEnabled(false);
USE_NATPMP is undefined. Did I understand you correctly?
Code review ACK 81a791d88d68e64968e52d8ff6ec20879bd5fad1
Really worthwhile change; all else equal the easier we can make offering inbound conn capacity the better.
I’ve reviewed and built locally, though my router doesn’t support NAT-PMP out of the box. I’ll see what I can do in the meantime in terms of more comprehensive testing.
Updated 81a791d88d68e64968e52d8ff6ec20879bd5fad1 -> cb4060686155c011cbece84fd551dfa000277d51 (pr18077.39 -> pr18077.40, diff).
Addressed @jamesob’s comments:
(Note on 1bf23f2 as a whole:) could be a scripteddiff AFAICT, but not a huge deal.
Nit: linebreak would be nice.
ACK https://github.com/bitcoin/bitcoin/commit/5fd127243f452827dab3b4f4f47b6e96816fdfb8
Re-reviewed diffs after rebase. Built locally with NAT-PMP enabled. My gateway doesn’t support NAT-PMP, but regular IBD works okay.
Updated 90d9547f135ef0ffc00dd790072c3ae307f053ad -> 29fb5708ca04da9ca0c25c1b801887354eb714bc (pr18077.43 -> pr18077.44, diff):
Perhaps I’m reading the code wrong, but it seems like “If both UPnP and NAT-PMP are enabled, a successful allocation from UPnP prevails over one from NAT-PMP” is a more accurate description her
286+ // Enabling another protocol does not cause switching from the currently used one.
287+ return;
288+ }
289+
290+ assert(g_mapport_thread.joinable());
291+ assert(!g_mapport_interrupt);
CThreadInterrupt class implementation.
In case any of this is helpful… I wanted to try running this, I’m running Ubuntu 18.04 as a Virtualbox guest on a Windows 10 host running at home (Xfinity-Comcast, nothing unusual), I checked out branch pr18077.47, ran
0$ sudo apt install libminiupnpc-dev libnatpmp-dev
1(... successful)
2$ ./autogen.sh && ./configure --disable-wallet --without-miniupnpc --enable-natpmp-default && make
3(...)
4checking whether to build with support for UPnP... no
5checking whether to build with support for NAT-PMP... yes
6checking whether to build with NAT-PMP enabled by default... yes
7(...)
8$ src/bitcoind
Starts up normally, but this prints to debug.log every 5 minutes:
02020-12-16T19:14:17Z natpmp: The gateway does not support NAT-PMP.
and bitcoin-cli -netstat shows that I have no incoming peers. I think this is all as expected, but I’m not testing much. I don’t know how to change my gateway to support NAT-PMP (I’d appreciate any help). I did change my cable modem router’s firewall to allow P2P applications on both IPv4 and IPv6, but that didn’t make any difference.
Thanks for testing!
I don’t know how to change my gateway to support NAT-PMP (I’d appreciate any help).
Your gateway s/w manual (if available) could help. My TP-Link Archer C6 does not have NAT-PMP support with stock s/w, so I’ve flashed the OpenWrt on it.
239+ if (ok) continue;
240+ }
241+#endif // #ifdef USE_UPNP
242+
243+#ifdef USE_NATPMP
244+ // Low priority protocol.
@luke-jr, what do you mean by “maps random ports often”? Some implementation of natpmp has bugs? Can you give an example?
Maybe just remove this comment “// Low priority protocol.” – it is unclear what is meant by it and is missing in other places where #ifdef USE_NATPMP is used.
@luke-jr, what do you mean by “maps random ports often”? Some implementation of natpmp has bugs? Can you give an example?
Some NAT-PMP daemon implementations do randomize an external port. For example, see #18077 (comment) and http://bodgitandscarper.co.uk/natpmpd/:
The external TCP or UDP port assigned to the client is always randomised rather than giving the first client the port it actually requested and then trying to work out what to do for additional clients that want the same external port.
@hebasto, Thanks for the clarification! IMO the nat-pmp implementation overriding the desired external port is fine as long as we advertise the overriden one instead of the desired one (I guess we do). That should also be expected as it is allowed by the RFC.
I guess something like that happens with UPnP too if the desired port is already used?
IMO the nat-pmp implementation overriding the desired external port is fine as long as we advertise the overriden one instead of the desired one (I guess we do).
Yes, we do: https://github.com/bitcoin/bitcoin/blob/890e59c64d62fd39778ede620a70fe6bc60d9993/src/mapport.cpp#L97-L102
I guess something like that happens with UPnP too if the desired port is already used?
It works !
libnatpmp-dev package, re-ran configure, it was detected correctly and enabled-natpmp -regtestI see the following in my log
02020-12-17T15:06:10Z natpmp: Port mapping successful. External address = x.x.x.x:18444
My router’s admin interface show the redirect:
0Protocol | External Port | Client Address | Client Port |
1-- | -- | -- | -- | --
2TCP | 18444 | 192.168.x.x | 18444
I have verified that I can connect to the host/port from a remote server.
17+#include <util/system.h>
18+
19+#ifdef USE_NATPMP
20+#include <natpmp.h>
21+#ifdef WIN32
22+#include <winsock2.h>
<compat.h>.
75+ } else if (r_read == NATPMP_ERR_NOGATEWAYSUPPORT) {
76+ LogPrintf("natpmp: The gateway does not support NAT-PMP.\n");
77+ } else {
78+ LogPrintf("natpmp: readnatpmpresponseorretry() for public address failed with %d error.\n", r_read);
79+ }
80+
216+ FreeUPNPUrls(&urls);
217+ } else {
218+ LogPrintf("No valid UPnP IGDs found\n");
219+ freeUPNPDevlist(devlist); devlist = nullptr;
220+ if (r != 0)
221+ FreeUPNPUrls(&urls);
0 if (r != 0) FreeUPNPUrls(&urls);
223+
224+ return ret;
225+}
226+#endif // #ifdef USE_UPNP
227+
228+static void ThreadMapPort()
Updated 93ff21a929ba3036d6cd5d822f300cf036ff939f -> 890e59c64d62fd39778ede620a70fe6bc60d9993 (pr18077.47 -> pr18077.48):
This commit does not change behavior.
-BEGIN VERIFY SCRIPT-
sed -i 's/g_upnp_interrupt/g_mapport_interrupt/' src/mapport.cpp
sed -i 's/if(g_upnp_thread/if (g_mapport_thread/' src/mapport.cpp
sed -i 's/g_upnp_thread/g_mapport_thread/' src/mapport.cpp
sed -i 's/LOCAL_UPNP/LOCAL_MAPPED/' src/mapport.cpp
sed -i 's/\bupnp\b/mapport/' src/mapport.cpp
sed -i 's/LOCAL_UPNP, /LOCAL_MAPPED,/' src/net.h
-END VERIFY SCRIPT-
This commit does not change behavior. It is a prerequisite for NAT-PMP
support adding.