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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #15890 (Doc: remove text about txes always relayed from -whitelist by harding)
• #15704 (Move Win32 defines to configure.ac to ensure they are globally defined by luke-jr)
• #15529 (Add Qt programs to msvc build by sipsorcery)
• #14856 (net: remove more CConnman globals (theuni) by dongcarl)

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:

1. Remove the copyright notices (we're licensed MIT)
2. Break the changes up into smaller, more digestible commits?

@dongcarl
Thanks.
Sure. Let me close this pull request and do three separate commits for those bullet points.

No need to close the PR; you can overwrite it by force pushing.

@practicalswift This is code copied from another project; unless we're going to submit improvements upstream, nits here are pointless.

@MishraShivendra Can you clarify which files/directories that are meant to be left as-is (with flaws reported upstream) and which files/directories that are not? :-)

@practicalswift It's literally in the PR description, and also obvious from the code (it's copyright someone else...)

@sipa The reason that I'm asking explicitly is that I noticed that some files added to this repo were not identical to the corresponding files in the repo that appears to be the upstream.

@MishraShivendra What is the correct upstream repo the files were copied from?

@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

src/natpmp/gateway.h seems to be missing the expected include guard:
  #ifndef BITCOIN_NATPMP_GATEWAY_H
  #define BITCOIN_NATPMP_GATEWAY_H
  ...
  #endif // BITCOIN_NATPMP_GATEWAY_H
src/natpmp/natmap_wrapper.h seems to be missing the expected include guard:
  #ifndef BITCOIN_NATPMP_NATMAP_WRAPPER_H
  #define BITCOIN_NATPMP_NATMAP_WRAPPER_H
  ...
  #endif // BITCOIN_NATPMP_NATMAP_WRAPPER_H
src/natpmp/natpmp.h seems to be missing the expected include guard:
  #ifndef BITCOIN_NATPMP_NATPMP_H
  #define BITCOIN_NATPMP_NATPMP_H
  ...
  #endif // BITCOIN_NATPMP_NATPMP_H
^---- 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)

I'm sorry about the confusion, https://github.com/miniupnp/libnatpmp is indeed the repo that's being updated.

@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:

1. A commit copying in the newest libnatpmp code
2. A commit integrating libnatpmp with bitcoin's build system
3. One or more commits integrating libnatpmp with bitcoin's codebase

Obviously there might be some mixing of 2 and 3, but at the very least 1 should be a separate commit.

There's no excuse to subtree this. It should be a normal shared library dependency.

Concept ACK in any case, thanks for picking this up.

@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.

There is already a separate shared library.

@dongcarl
Travis-ci build seems to be inconsistent. The above PR build seems to fail but mainline of my forked repo passes: Travis-Ci-Master-Build.
Can someone please restart the PR build?

@practicalswift The current commit includes most of the changes you suggested. Can you please take a look?

@luke-jr Can you please point me to it. I couldn't find any place where we already build one. Did you mean upnp shared lib?

@MishraShivendra Sure! In light of previous comments -- do you want me to review all the files or just a subset? In the latter case, what subset? :-)

@practicalswift
A quick look at everything, please.
To shed some light on the changes:
These (first commit) are the file belong to libnatpmp and this diff shows the changes I've introduced in upstream.
Second commit introduces integration changes. net.cpp includes changes to start/stop/renew the port mapping. RenewPortMapping() is a detached thread to periodically renew the port mapping. Feel free to raise any questions :)

@MishaShivendra It's installed with the user's OS.

Pushed suggested changes.

Can someone please give input in the above?

You should leave UPnP stuff alone for this. Just add NAT-PMP support as a separate feature.

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:

1. NAT-PMP should be enabled by default
   2. Let's not submit more reviews of the subtree'd code, as we might make it a dependency under depends
2. Updated: We are going to make NAT-PMP part of the bitcoind 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:

gArgs.AddArg("-upnpc_bin", "Path to port forwarding client binary.");
...
if (gArgs.IsArgSet("-upnp_bin")) {
   bp::system(strprintf("%s %i %i", gArgs.GetArg("-upnp_bin"), internal_port, external_port));
}

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:

1. Leave UPnP as is and keep nat-pmp as a separate feature: Would introduce "-natpmp" switch to enable, and absense of this switch would disable nat-pmp port mapping
2. Sub-tree libnatpmp changes: Would sub-tree libnatpmp. I would remove all unused files and maintain the same gateway/natpmp (.cpp/.h) files introduced already in this PR.
3. Anything else.

Let me know if you all agree with the aforementioned.

That's not what I meant. Just use standard pkg-config for libnatpmp. Every major distro has it already.

1. Yes
2. What Luke says, plus adding to depends because we need that for distribution builds (though that can wait for a followup if if the feature is off by default)

@luke-jr @Sjors
So you mean to say #2 should be:
2. Sub-tree libnatpmp changes: Would sub-tree libnatpmp. I would remove all unused files and maintain the same gateway/natpmp (.cpp/.h) files introduced already in this PR. Use package-config for libnatpmp and add depends.

No, I would not sub-tree libnatpmp. Just rely on 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...

@luke-jr
I know that la-files are obsolete, but as libnatpmp has no pkg-config, there was no other option. Maybe there is one that I don't know about?

Anyway, I consider this task as obsolete then.

Thanks.

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).