[WIP][NET] Add NATPMP support. #12288

annanay25 commented at 8:15 PM on January 28, 2018: none

Adds minimal support for NATPMP (and removes dependency on external UPnP library). Reference implementation in miniupnp/libnatpmp.

WIP but aims to fix #11902 .

Add basic NATPMP support, get gateway by file parsing, getpublicaddress, map public-private port 529f3931dd

Add license info on some files d4d3c05bb2

TheBlueMatt commented at 8:37 PM on January 28, 2018: member

I assume libnatpmp should be subtree'd or we should still use the version linked-against in miniupnpc?

On January 28, 2018 8:15:18 PM UTC, Annanay Agarwal notifications@github.com wrote:

Adds minimal support for NATPMP (and removes dependency on external UPnP library). Reference implementation in miniupnp/libnatpmp.

WIP but aims to fix #11902 . You can view, comment on, or merge this pull request online at:

#12288

-- Commit Summary --

Add basic NATPMP support, get gateway by file parsing, getpublicaddress, map public-private port

-- File Changes --

A src/getgateway.c (570) A src/getgateway.h (47) A src/natpmp.c (354) A src/natpmp.h (182) M src/net.cpp (187) A src/wingettimeofday.c (60) A src/wingettimeofday.h (36)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/12288.patch https://github.com/bitcoin/bitcoin/pull/12288.diff

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/12288

sipa commented at 8:42 PM on January 28, 2018: member

This PR just copies 6 files from libnatpmp into the repository. Perhaps this is a sufficiently low amount of code that we want to do this (like with tinyformat), but if so, putting them in a subdirectory together may be better.

I haven't checked how much changes were necessary to them, but if it was 0, perhaps we can subtree but still build them directly from our build system (as opposed to building as a library and linking statically).

fanquake added the label P2P on Jan 29, 2018

annanay25 commented at 5:45 AM on January 29, 2018: none

There are minor changes to the copied files, some including changing the WIN32 macro, and others according to the coding guidelines like changing NULL to nullptr, etc.

fanquake deleted a comment on Jan 29, 2018

laanwj commented at 9:02 AM on January 29, 2018: member

libnatpmp seems to have been virtually unmaintained a while (last commit was Mar 15, 2016), so I think it doesn't matter whether we use 'subtree'. It might be easier to just include the (small) implementation files into our build system and see if we can maintain it ourselves, cutting out parts we don't need.

At least now we don't have to carry the .javas.

putting them in a subdirectory together may be better.

I agree with that - whatever you do, please don't put them in src/ root.

laanwj assigned laanwj on Feb 8, 2018

laanwj commented at 4:06 PM on February 8, 2018: member

This is failing the travis build - did you perhaps forget to add natpmp.h to the makefile, so that it's not included in make dist?

Making check in src
make[1]: Entering directory `/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-w64-mingw32/src'
make[2]: Entering directory `/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-w64-mingw32/src'
  CXX      libbitcoin_server_a-net.o
  CXX      libbitcoin_server_a-noui.o
  CXX      libbitcoin_server_a-pow.o
  CXX      libbitcoin_server_a-rest.o
net.cpp:31:20: fatal error: natpmp.h: No such file or directory
 #include <natpmp.h>
                    ^
compilation terminated.
make[2]: *** [libbitcoin_server_a-net.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-w64-mingw32/src'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-w64-mingw32/src'
make: *** [check-recursive] Error 1

laanwj unassigned laanwj on Mar 5, 2018

Added natpmp.h to Makefile and moved files to subdirectory 9fb118e122

Added natpmp/natpmp.h to Makefile.am dae9e0f6b6

Merge branch 'master' into natpmp-support 1a4ed7425c

Added natpmp_declspec.h and changed some naming conventions 25da431c18

Rebased, resolved conflicts 3b730b1c83

Merged on GH web. Resolving now.

Merge branch 'natpmp-support' of https://github.com/annanay25/bitcoin into natpmp-support

baddfa1d4e

Add natpmp/natpmp_declspec.h to Makefile.am e42be34411

Fix headers 883ff11134

Updated StartMapPort() 2e8c84f18c

Fixed net.cpp errors c663b6f067

Added dependency files ec5158c00e

Some changes to work with NATPMP API 3e70af696e

Rebasing

Merge branch 'master' into natpmp-support

2476d2856b

Fix includes 14547d17ef

Fix includes again 74dc8f73fb

Added more headers to Makefile e1ca330320

Added more headers to Makefile fd6eae66d8

annanay25 commented at 7:36 AM on March 13, 2018: none

@laanwj ;

I'm a little confused with the name mangling happening here. The src/natpmp/*.c files have extern "C" in the function prototypes, which confirms no name mangling (confirmed this with nm src/natpmp/libbitcoin_server_a-natpmp.o | grep initnatpmp), but C++ object files are looking for mangled function names?

Add atoi for port mapping a9c9b10cf4

laanwj commented at 9:59 AM on April 11, 2018: member

but C++ object files are looking for mangled function names?

You need the extern "C" in the header, not in the implementation files.

Fix extern C in header files fcd026b8e0

DrahtBot closed this on Jul 21, 2018

DrahtBot commented at 5:29 PM on July 21, 2018: member

The last travis run for this pull request was 101 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

DrahtBot reopened this on Jul 21, 2018

luke-jr changes_requested

luke-jr commented at 5:50 PM on July 21, 2018: member

Libraries should not be copied or subtree'd. There's arguably an excuse for consensus-critical libraries, but NATPMP is not that. Link to the system library if present, and if not, just don't build support for it.

in src/natpmp/natpmp.h:129 in fcd026b8e0

 124 | +extern "C" {
 125 | +#endif
 126 | +
 127 | +/* initnatpmp()
 128 | + * initialize a natpmp_t object
 129 | + * With forcegw=1 the gateway is not detected automaticaly.

practicalswift commented at 6:21 PM on September 2, 2018:

Typo found by codespell: automaticaly

in src/net.cpp:1550 in fcd026b8e0

1609 | -        LogPrintf("No valid UPnP IGDs found\n");
1610 | -        freeUPNPDevlist(devlist); devlist = nullptr;
1611 | -        if (r != 0)
1612 | -            FreeUPNPUrls(&urls);
1613 | +        LogPrintf("Mapped public port %hu protocol %s to local port %hu "
1614 | +                "liftime %u\n",

practicalswift commented at 6:22 PM on September 2, 2018:

Typo found by codespell: liftime

DrahtBot commented at 1:26 PM on September 21, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14856 ([WIP] 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.

practicalswift commented at 7:22 PM on December 9, 2018: contributor

@annanay25 It seems like the work on this PR has stalled somewhat. Do you have time to continue working on this PR? :-)

annanay25 commented at 7:11 AM on December 10, 2018: none

Is this PR still relevant? I'm not sure if I will be able to continue working on it. @practicalswift

laanwj commented at 7:23 AM on December 10, 2018: member

I think it's still relevant, but closing if you're not able to work on it anymore. Will add "up for grabs" tag.

laanwj closed this on Dec 10, 2018

laanwj added the label Up for grabs on Dec 10, 2018

MishraShivendra commented at 8:01 PM on March 23, 2019: none

Can someone please re-assign #11902 to me? I would like to take this PR forward.

fanquake removed the label Up for grabs on Feb 6, 2020

laanwj referenced this in commit d7e2401c62 on Jan 7, 2021

DrahtBot locked this on Feb 15, 2022