Skip to content
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.

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

[Net] Add NAT-PMP port forwarding support #2338

Merged
merged 19 commits into from
Jun 24, 2021

Conversation

Fuzzbawls
Copy link
Collaborator

@Fuzzbawls Fuzzbawls commented Apr 24, 2021

Built on top of #2330, this backports bitcoin#18077 and bitcoin#21209 to add NAT-PMP port forwarding support, which can function along side of or instead of UPnP.

Similar to how UPnP is treated, NAT-PMP support will be compiled in if the library is found, but the functionality is disabled by default unless passing --enable-natpmp-default to configure, or by using the -natpmp command line option at startup.

A checkbox has also been added to the settings area of the GUI that allows for on-the-fly enabling/disabling of the port mapping features when saving the settings.

@Fuzzbawls
Copy link
Collaborator Author

rebased now that #2330 has been merged, ready for review.

@Fuzzbawls Fuzzbawls requested review from random-zebra and furszy and removed request for random-zebra May 17, 2021 23:04
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Code review and lightly tested ACK.
Just few minor discussion points for the GUI controls.

src/qt/pivx/settings/settingswalletoptionswidget.cpp Outdated Show resolved Hide resolved
src/init.cpp Show resolved Hide resolved
src/qt/pivx/settings/settingswalletoptionswidget.cpp Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
doc/release-notes.md Outdated Show resolved Hide resolved
@Fuzzbawls
Copy link
Collaborator Author

rebased to resolve conflicts and addressed feedback

random-zebra
random-zebra previously approved these changes Jun 4, 2021
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Code ACK 5876841

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code review 5876841.

src/qt/clientmodel.h Outdated Show resolved Hide resolved
src/qt/pivx/settings/settingswalletoptionswidget.cpp Outdated Show resolved Hide resolved
Fuzzbawls and others added 9 commits June 13, 2021 23:23
This commit does not change behavior.
# Conflicts:
#	src/mapport.cpp
-BEGIN VERIFY SCRIPT-
sed -i 's/g_upnp_interrupt/g_mapport_interrupt/' 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.
fanquake and others added 2 commits June 16, 2021 12:26
The source we are currently using is from 2015. The upstream repo has
received a small number of bug fixes and improvements since then.
Including one that fixes an issue for Windows users:
miniupnp/libnatpmp#13.

The source we are currently using is the most recent "official" release,
however I don't think it's worth waiting for a new one. The maintainer
was prompted to do so in Oct 2020, then again in Jan of this year, and
no release has eventuated. Given libnatpmp is a new inclusion into our
repository, I think we should be using this newer source.

This also cleans up a few warnings we currently see in depends builds:
```bash
Extracting libnatpmp...
/home/ubuntu/bitcoin/depends/sources/libnatpmp-20150609.tar.gz: OK
Preprocessing libnatpmp...
Configuring libnatpmp...
Building libnatpmp...
make[1]: Entering directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87'
x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR   -c -o natpmp.o natpmp.c
x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR   -c -o getgateway.o getgateway.c
natpmp.c:42: warning: "EWOULDBLOCK" redefined
   42 | #define EWOULDBLOCK WSAEWOULDBLOCK
      |
In file included from natpmp.c:38:
/usr/share/mingw-w64/include/errno.h:166: note: this is the location of the previous definition
  166 | #define EWOULDBLOCK 140
      |
natpmp.c:43: warning: "ECONNREFUSED" redefined
   43 | #define ECONNREFUSED WSAECONNREFUSED
      |
In file included from natpmp.c:38:
/usr/share/mingw-w64/include/errno.h:110: note: this is the location of the previous definition
  110 | #define ECONNREFUSED 107
      |
natpmp.c:271:5: warning: ‘readnatpmpresponseorretry’ redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
  271 | int readnatpmpresponseorretry(natpmp_t * p, natpmpresp_t * response)
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
ar crs libnatpmp.a natpmp.o getgateway.o
make[1]: Leaving directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87'
Staging libnatpmp...
Postprocessing libnatpmp...
Caching libnatpmp...
```
This fixes linking issues and mirrors what we do with miniupnpc.
@@ -16,7 +16,7 @@ Then install [Homebrew](https://brew.sh).
Dependencies
----------------------

brew install autoconf automake berkeley-db4 libtool boost miniupnpc pkg-config python3 qt5 zmq libevent qrencode gmp libsodium rust
brew install autoconf automake berkeley-db4 libtool boost miniupnpc libnatpmp pkg-config python3 qt5 zmq libevent qrencode gmp libsodium rust
Copy link

Choose a reason for hiding this comment

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

future nit: miniupnpc and libnatpmp are optional.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

upnp and code check ACK c198797.

Left a tiny nit that can definitely be tackled in another PR.

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK c198797 after rebase, and merging...

@random-zebra random-zebra merged commit fce12cf into PIVX-Project:master Jun 24, 2021
@Aeinsteroid54
Copy link

****Aeinsteroid54/BEPs: [NET] Add NAT-PMP post forwarding support#2337

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants