Changes to support NAT-PMP port forwarding based on libnatpmp to address #11902. Changes include the following:
Test
0$./src/bitcoind -portmap
Note to reviewers are here, need insights.
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.
@MishraShivendra I would love to review this, would you be able to do the following:
81+
82+ if(returnCode<0) {
83+ return returnCode;
84+ }
85+
86+ publicPortOut = response.pnu.newportmapping.mappedpublicport,
; and not , right? :-)
@practicalswift Thanks for reviewing the changes. Those files were taken from here. I’ve taken just the files needed. A few headers were merged together and useless code was removed. A few other error codes were also added. Please see diff between corresponding .c and .h files. Would push suggested changes in a while.
@practicalswift This is code copied from another project; unless we’re going to submit improvements upstream, nits here are pointless. @sipa I think we can fix/modify based on our need. Those files are already way too customized for upstream. @dongcarl Removing license headers will lead to writing implementation of our own. Not sure if that would be a good idea. Any input?
travis linter complains; either this needs the expected include guards, or to be excluded from the lints
0src/natpmp/gateway.h seems to be missing the expected include guard:
1 #ifndef BITCOIN_NATPMP_GATEWAY_H
2 #define BITCOIN_NATPMP_GATEWAY_H
3 ...
4 #endif // BITCOIN_NATPMP_GATEWAY_H
5src/natpmp/natmap_wrapper.h seems to be missing the expected include guard:
6 #ifndef BITCOIN_NATPMP_NATMAP_WRAPPER_H
7 #define BITCOIN_NATPMP_NATMAP_WRAPPER_H
8 ...
9 #endif // BITCOIN_NATPMP_NATMAP_WRAPPER_H
10src/natpmp/natpmp.h seems to be missing the expected include guard:
11 #ifndef BITCOIN_NATPMP_NATPMP_H
12 #define BITCOIN_NATPMP_NATPMP_H
13 ...
14 #endif // BITCOIN_NATPMP_NATPMP_H
15^---- failure generated from test/lint/lint-include-guards.sh
Remove the copyright notices (we’re licensed MIT)
I don’t think this is allowed, at least the 3-clause BSD license has more restrictions than the MIT one.
Ok, we discussed the license issue on IRC.
The conclusion was that it’s probably best to mention the differing license for the files in contrib/debian/copyright (per file), so that people can see this in one glance without having to view the individual source files, but apart from that it’s acceptable to include them. 3BSD versus MIT is close enough, and is not expected to cause problems.
Also, I believe the upstream is really here, I’m not sure if there’s a diff between what you copied and what’s available upstream, so checking would be great!
I was wrong, see comment here: #15717 (comment)
@MishraShivendra It seems that Travis is failing. Could you take a look at the errors there?
Aside from that, please rearrange your commits so they look something like this:
Obviously there might be some mixing of 2 and 3, but at the very least 1 should be a separate commit.
@dongcarl @laanwj Thanks for those input and clarification.
It seems that Travis is failing. Could you take a look at the errors there?
Changes already in progress on a private branch. I’ll push them shortly.
There’s no excuse to subtree this. It should be a normal shared library dependency. @luke-jr I believe it was already discussed in #12288 . Not sure if we need a separate shared library for these changes.? I’m including 2 files from libnatpmp though.
* Pulls core NAT-PMP files from https://github.com/miniupnp/libnatpmp.git
* Above codes are ported/filtered for easier integration.
76+ NONE,
77+ INITIALIZED,
78+ RECEIVED_PUBLIC_IP,
79+ MAPPED_PORT
80+};
81+typedef enum NatPmpState NatPmpStateType;
using instead of typedef.
33+/// @brief Wrapper over libnatpmp functions
34+class NatMap
35+{
36+
37+private:
38+ natpmp_t m_NatPmpObj;
m_NatPmpObj here or in the constructor. As the code is currently formulated m_NatPmpObj will remain uninitialised if the InitNatPmp call in the constructor fails.
9+ m_State = InitNatPmp(&m_NatPmpObj,
10+ alternateGWAddr?1:0,
11+ gateway);
12+}
13+
14+int NatMap::IsGood(void) const
void redundant.
290+ p->has_pending_request = 0;
291+ }
292+ return n;
293+}
294+
295+const char * StrNatPmpErr(int r)
58+#else
59+ int flags;
60+#endif
61+ if(!p)
62+ return NATPMP_ERR_INVALIDARGS;
63+ struct sockaddr_in addr;
40+
41+public:
42+ /// @brief Constructor
43+ ///
44+ /// @param alternateGWAddr Pass a known gateway if default not to be used.
45+ NatMap(in_addr_t* alternateGWAddr = nullptr);
explicit :-)
362+ // -DhcpIPAddress = 10.0.1.4
363+ // -DhcpDefaultGateway = 10.0.1.1
364+ // -[more]
365+ // -{86226414-5545-4335-A9D1-5BD7120119AD}
366+ // -DhcpIpAddress = 10.0.1.5
367+ // -DhcpDefaultGateay = 10.0.1.1
118+ if(!p)
119+ return NATPMP_ERR_INVALIDARGS;
120+ p->has_pending_request = 1;
121+ p->try_number = 1;
122+ int n = sendpendingrequest(p);
123+ gettimeofday(&p->retry_time, nullptr);
gettimeofday is checked and in some parts it is not. Consider doing it consistently throughout this PR: either the assumption is that it can fail or the assumption is that it cannot :-)
121+*/
122+
123+int TestIfWhiteSpace(char character)
124+{
125+ std::list<char> whiteSpaces = {'\f', '\n', '\r', '\t', '\v'};
126+ return std::count(whiteSpaces.begin(), whiteSpaces.end(), character);
IsSpace(char c) (strencodings.h).
134+ int line = 0;
135+ char * p = nullptr;
136+ FILE* f = fopen("/proc/net/route", "r");
137+ if(!f)
138+ return FAILED;
139+ while(fgets(buf, sizeof(buf), f)) {
clang-tidy on all new files. For changes to existing files: use https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/clang-format-diff.py
* Changes to support NAT-PMP based on libnatpmp
* Introduces wrapper over libnatpmp for easier integration
* Changes to disable upnp by default and use NAT-PMP
as explained in #11902 (Introducing -portmap switch)
* Integration of above in build system
436@@ -437,16 +437,17 @@ void SetupServerArgs()
437 gArgs.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", false, OptionsCategory::CONNECTION);
438 #ifdef USE_UPNP
439 #if USE_UPNP
440- gArgs.AddArg("-upnp", "Use UPnP to map the listening port (default: 1 when listening and no -proxy)", false, OptionsCategory::CONNECTION);
441+ gArgs.AddArg("-portmap", "Use UPnP to map the listening port (default: 1 when listening and no -proxy)", false, OptionsCategory::CONNECTION);
442 #else
443- gArgs.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %u)", 0), false, OptionsCategory::CONNECTION);
444+ gArgs.AddArg("-portmap", strprintf("Use NAT-PMP to map the listening port (default: %u)", 0), false, OptionsCategory::CONNECTION);
445 #endif
USE_UPNP is a compile-time option, not run-time.
UPnP and NAT-PMP are two completely different protocols. They don’t do the same thing. Some routers may support only one or the other.
There was some discussion about this in IRC yesterday:
http://www.erisian.com.au/bitcoin-core-dev/log-2019-04-24.html#l-255
I did want to throw out as a suggestion that instead of running this code inside bitcoind or even adding it to the project we could consider calling out to a command line client with boost::process like: https://superuser.com/questions/192132/how-to-automatically-forward-a-port-from-the-router-to-a-mac-upnp/192487#192487. This could potentially be a very small code change after #15382 which adds boost::process. But this is just a thought which doesn’t need to affect the current effort.
There was some discussion about this in IRC yesterday:
http://www.erisian.com.au/bitcoin-core-dev/log-2019-04-24.html#l-255
Yes that discussion resulted in a few conclusions:
dependsbitcoind codebase, exactly like you’ve done here (IRC conversation)w/re how we’re going to depend on libnatpmp, my naive thinking is that it should be a dependency under depends that we statically link into our binaries, since it needs to be on by default. Maybe this doesn’t make sense, and others can advise.
I did want to throw out as a suggestion that instead of running this code inside bitcoind or even adding it to the project we could consider calling out to a command line client with boost::process like:
Please, no scope creep here. This can always be done later but there’s no need to complicate this effort even further with a requirement that it should run outside the process.
Updated: We are going to make NAT-PMP part of the bitcoind codebase, exactly like you’ve done here
NACK. There is no excuse for this. It is universally held to be a bad practice in the open source community, and for very good reasons. We have an excuse to bundle consensus-critical stuff because upstream doesn’t generally consider the ramifications of some bugfixes, but that does not apply to NAT-PMP at all.
Please, no scope creep here.
Just in case there is a misunderstanding, the change I was referring to is potentially a very small change after #15382. Something like:
0gArgs.AddArg("-upnpc_bin", "Path to port forwarding client binary.");
1...
2if (gArgs.IsArgSet("-upnp_bin")) {
3 bp::system(strprintf("%s %i %i", gArgs.GetArg("-upnp_bin"), internal_port, external_port));
4}
But I mostly just wanted to suggest it an alternative to the approaches discussed above (which all seem perfectly fine to me).
Concept ACK. I’m OK with compiling this by default
I don’t think we should turn this on by default until the GUI is updated: that’s better done in a followup.
I’m reluctant to strip UPnP compilation by default. The GUI currently has a “map port using UPnP” (off by default). Although it’s tempting to just change the text and meaning of that textbox, that’s a can of worms by itself. It seems better to just add a checkbox for this new setting, and mention in the tooltip which one is recommended.
I tend to agree with @luke-jr on avoiding pulling in code. The Depends system seems more appropriate. From IRC in favor of subtree’ing:
the main reason for not using it as a library was because the upstream library looked to be unmaintained, or at least have no new releases for years
The last Homebrew release is from 2015. Ditto for Ubuntu Bionic. I haven’t tested those releases. If they have problems then we could encourage the author to issue a new release. Also, a slight lazy argument of why fresh releases aren’t super important, is that anyone who can compile from source can learn how to forward a port; this library primarily benefits users of the binary distribution.
There is still is some modest commit activity, so we shouldn’t declare it dead upstream. Even if it was, I would prefer forking the repo in the bitcoin-core org. The act us of using it could also bring new life to the project. In fact I think we should encourage that, since a lot projects stand to benefit from this library.
For a followup I like the idea of using boost::process, but as @laanwj says let’s not complicate this PR. It could be a while before #15382 is merged too.
I did want to throw out as a suggestion that instead of running this code inside bitcoind or even adding it to the project we could consider calling out to a command line client with boost::process like:
I don’t seem to agree. It could be difficult to find these errors through running the binary with the help of boost::process.
I happen to agree with @luke-jr idea to sub-tree libnatpmp related changes.
Just to sum it up:
Let me know if you all agree with the aforementioned.
pkg-config for libnatpmp. Every major distro has it already.
pkg-config to find libnatpmp on the system (during ./configure). The wrapper you created can go in src/util or some similar place. Don’t worry about depends: that can wait for a future commit.
Hi,
First, I’m not sure if this task is still “up for grabs”, because it’s closed now. However, the icon is still there, so it should be. Anyway, I’m working on it out of curiosity, because I only know about UPnPC, so I thought it would be nice to learn how NAT-PMP works. :)
The commit in my forked repo contains following changes:
Integrated libnatpmp source directory (similar to secp256k1 and univalue)
Created autogen.sh, configure.ac and Makefile.am for libnatpmp, because the original repo doesn’t have them. Instead, they offer a Makefile which makes it non-portable (no .la-files generation available).
Added a few build-aux/m4 macros to search for pthreads, compiler & linker flags, and other stuff.
Bitcoin’s configure.ac was changed to allow for searching of system libnatpmp or to build it from source. However, as libnatpmp has no pkg-config, the search is not the same as with univalue. Instead, it simply tries to find natpmp.h include file. Not sure, if this is enough for every OS, but we could maybe test it.
Adapted src/Makefile.am to build and link libnatpmp. Also adapted: Makefile.qt.include and Makefile.test.include
Added src/natmap.cpp and src/natmap.h where we access the libnatpmp API. The code in these two files is partially based on the example code from libnatpmp webpage as well as the code from the initial poster of this PR. However, I have removed the whole C-to-C++ mapping stuff, because the C code from libnatpmp is so horrible that there is practically no good way to abstract it away. Instead, I’ve simply taken all the ugly only-lowercase-because-I-hate-humans-Functions and put them directly into NatMap class. YOLO! Anyway, all kudos go to libnatpmp developer!
Well, that’s it. I don’t know the current state of this task, but in any case, it was fun writing configure scripts, trying out various Makefile settings,…and reading through ugly C code.
Regards,
Integrated libnatpmp source directory (similar to secp256k1 and univalue)
Don’t do this.
non-portable (no .la-files generation available).
.la files are generally considered obsolete…
Adding. NAT-PMP support is not obsolete, see #11902. The question is just how.
I think @luke-jr’s point about integrating the source directory is that this practice should be avoided, especially for projects that are still being (somewhat) maintained. It’s better to make a PR upstream to add pkg-config support.
Meanwhile you could add it to https://github.com/bitcoin/bitcoin/tree/master/depends, which works by fetching the code from its original source, and has room for patches. The only problem there is that afaik you can’t build 1 depends package and get the rest via pkg-config (but I could be wrong).