Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 21 commits into from
Closed

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

wants to merge 21 commits into from

Conversation

annanay25
Copy link

@annanay25 annanay25 commented Jan 28, 2018 •
edited by DrahtBot
Loading

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

WIP but aims to fix #11902 .

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Jan 28, 2018 via email

@sipa
Copy link
Member

sipa commented Jan 28, 2018 •
edited
Loading

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 fanquake added the P2P label Jan 29, 2018
@annanay25
Copy link
Author

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.

@bitcoin bitcoin deleted a comment Jan 29, 2018
@bitcoin bitcoin deleted a comment Jan 29, 2018
@laanwj
Copy link
Member

laanwj commented Jan 29, 2018

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 laanwj self-assigned this Feb 8, 2018
@laanwj
Copy link
Member

laanwj commented Feb 8, 2018

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 laanwj removed their assignment Mar 5, 2018
annanay25 and others added 17 commits March 9, 2018 14:41
@annanay25
Copy link
Author

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

@laanwj
Copy link
Member

laanwj commented Apr 11, 2018

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

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

@DrahtBot DrahtBot closed this Jul 21, 2018
@DrahtBot
Copy link
Contributor

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 DrahtBot reopened this Jul 21, 2018
luke-jr
luke-jr suggested changes Jul 21, 2018
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DrahtBot DrahtBot mentioned this pull request Jul 30, 2018
Closed
This was referenced Aug 23, 2018
Closed
Closed
practicalswift
practicalswift reviewed Sep 2, 2018
View reviewed changes
src/natpmp/natpmp.h

/* initnatpmp()
* initialize a natpmp_t object
* With forcegw=1 the gateway is not detected automaticaly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo found by codespell: automaticaly

practicalswift
practicalswift reviewed Sep 2, 2018
View reviewed changes
src/net.cpp
if (r != 0)
FreeUPNPUrls(&urls);
LogPrintf("Mapped public port %hu protocol %s to local port %hu "
"liftime %u\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo found by codespell: liftime

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 21, 2018 •
edited
Loading

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
Copy link
Contributor

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

@annanay25
Copy link
Author

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

@laanwj
Copy link
Member

laanwj commented Dec 10, 2018

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 laanwj closed this Dec 10, 2018
@MishraShivendra
Copy link

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

@MishraShivendra MishraShivendra mentioned this pull request Apr 10, 2019
Closed
@hebasto hebasto mentioned this pull request Feb 5, 2020
Merged
laanwj added a commit that referenced this pull request Jan 7, 2021
@laanwj
Merge #18077: net: Add NAT-PMP port forwarding support
d7e2401 
a191e23 doc: Add release notes (Hennadii Stepanov)
ae749d1 doc: Add libnatpmp stuff (Hennadii Stepanov)
e28f9be ci: Add libnatpmp-dev package to some builds (Hennadii Stepanov)
5a0185b gui: Add NAT-PMP network option (Hennadii Stepanov)
a39f733 net: Add -natpmp command line option (Hennadii Stepanov)
28acffd net: Add NAT-PMP to port mapping loop (Hennadii Stepanov)
a8d9f27 net: Add libnatpmp support (Hennadii Stepanov)
58e8364 gui: Apply port mapping changes on dialog exit (Hennadii Stepanov)
cf151cc scripted-diff: Rename UPnP stuff (Hennadii Stepanov)
4e91b1e net: Add flags for port mapping protocols (Hennadii Stepanov)
8b50d1b net: Keep trying to use UPnP when -upnp=1 (Hennadii Stepanov)
28e2961 refactor: Replace magic number with named constant (Hennadii Stepanov)
02ccf69 refactor: Move port mapping code to its own module (Hennadii Stepanov)

Pull request description:

  Close #11902
  This PR is an alternative to:
  - #12288
  - #15717

  To compile with NAT-PMP support on Ubuntu [`libnatpmp-dev`](https://packages.ubuntu.com/source/bionic/libnatpmp) should be available.

  Log excerpt:
  ```
  2020-02-05T20:12:28Z [mapport] NAT-PMP: public address = 95.164.65.194
  2020-02-05T20:12:28Z [mapport] AddLocal(95.164.65.194:18333,3)
  2020-02-05T20:12:28Z [mapport] NAT-PMP: port mapping successful.
  ```

  See: [`libnatpmp`](https://miniupnp.tuxfamily.org/libnatpmp.html)

  ---

  Some follow-ups are out of this PR's scope:
  - mention NAT-PMP library in the version message
  - ~integrate NAT-PMP into the GUI~ (already [added](#18077 (comment)))

ACKs for top commit:
  laanwj:
    Tested and code review ACK a191e23

Tree-SHA512: 10e19267c21bf30f20ff1abfc882d526049f0e790b95e12f109dc2bed7c0aef45de03eaf967f4e667e7509be04f1873a5c508087393d947205f3aab2ad6d7cf1
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@practicalswift practicalswift practicalswift left review comments

@luke-jr luke-jr luke-jr requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NAT-PMP port forwarding support
9 participants
@annanay25 @TheBlueMatt @sipa @laanwj @DrahtBot @practicalswift @MishraShivendra @luke-jr @fanquake