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 #18077

Merged
merged 13 commits into from Jan 7, 2021
Merged

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 5, 2020

Close #11902
This PR is an alternative to:

To compile with NAT-PMP support on Ubuntu libnatpmp-dev 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


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)

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Feb 5, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #20864 (net: Move SocketSendData lock annotation to header by MarcoFalke)
  • #20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
  • #20487 (draft: Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
  • #20201 (build: pkg-config related cleanup by hebasto)
  • #20196 (net: fix GetListenPort() to derive the proper port by vasild)
  • #19683 (depends: Pin clang search paths for darwin host by dongcarl)
  • #19064 (refactor: Cleanup thread ctor calls by hebasto)
  • #17920 (guix: Build support for macOS by dongcarl)
  • #16546 (External signer support - Wallet Box edition by Sjors)

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.

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 6, 2020

Thanks for picking this up again. I hope we manage to do this this time.

src/mapport.cpp Outdated Show resolved Hide resolved
@luke-jr
Copy link
Member

@luke-jr luke-jr commented Feb 6, 2020

Concept ACK

@hebasto hebasto marked this pull request as ready for review Feb 7, 2020
@hebasto hebasto changed the title [WIP] net: Add NAT-PMP port forwarding support net: Add NAT-PMP port forwarding support Feb 7, 2020
@hebasto
Copy link
Member Author

@hebasto hebasto commented Feb 7, 2020

It is ready for reviewing now.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Feb 7, 2020

Updated 4ff5349 -> cf94431 (pr18077.03 -> pr18077.04, compare):

added libnatpmp package to macOS 10.14 native Travis build.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Feb 8, 2020

Updated cf94431 -> 88248a4 (pr18077.04 -> pr18077.05, compare):

added:

  • changes to docs
  • release notes

@hebasto
Copy link
Member Author

@hebasto hebasto commented Feb 10, 2020

Rebased after #17398 and #18081 have been merged.

@dongcarl dongcarl self-assigned this Feb 10, 2020
@dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Feb 10, 2020

Concept ACK, will test on my home router.

@Sjors
Copy link
Member

@Sjors Sjors commented Feb 11, 2020

Concept ACK. Lightly tested configure / build on macOS.

Unfortunately I'm behind a IPv6 DS-Lite router, I can't test the IPv4 port forward. Perhaps -7 could be replaced by something more readable.

2020-02-11T17:48:27Z natpmp thread start
2020-02-11T17:48:27Z Bound to [::]:8333
2020-02-11T17:48:27Z Bound to 0.0.0.0:8333
2020-02-11T17:48:27Z init message: Loading P2P addresses...
2020-02-11T17:48:27Z NAT-PMP: readnatpmpresponseorretry() for public address failed with -7 error.
2020-02-11T17:48:27Z NAT-PMP: readnatpmpresponseorretry() for port mapping failed with -7 error. 

I looked up the error code, and it's what I would expect given my setup:

/* NATPMP_ERR_NOGATEWAYSUPPORT : the gateway does not support NAT-PMP */
#define NATPMP_ERR_NOGATEWAYSUPPORT (-7)

Two suggesitons for followups, or this PR:

  • add checkbox in QT Settings (above UPNP)
  • add support for IPv6 pinhole if the library supports it, see #17012 (looks like it doesn't)

@hebasto
Copy link
Member Author

@hebasto hebasto commented Feb 11, 2020

@Sjors

Thank you for your review and testing.

I looked up the error code, and it's what I would expect given my setup:

/* NATPMP_ERR_NOGATEWAYSUPPORT : the gateway does not support NAT-PMP */
#define NATPMP_ERR_NOGATEWAYSUPPORT (-7)

Agree.
But in this initial PR I intentionally do not provide user-friendly error messages in order to keep ThreadNatpmp() dense and tight for easier reviewing ;)

In followups every library call (initnatpmp(), sendpublicaddressrequest() and sendnewportmappingrequest()) with the following readnatpmpresponseorretry() loops could be refactored out to separate functions with user-friendly error messages.

Two suggesitons for followups, or this PR:

* add checkbox in QT Settings (above UPNP)

Agree. From the OP:

Some follow-ups are out of this PR's scope:

* mention NAT-PMP library in the version message

* integrate NAT-PMP into the GUI

@hebasto hebasto deleted the 20200130-natpmp branch Jan 7, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jan 7, 2021
@@ -50,6 +51,10 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
#ifndef USE_UPNP
ui->mapPortUpnp->setEnabled(false);
#endif
connect(this, &QDialog::accepted, [this](){
QSettings settings;
model->node().mapPort(settings.value("fUseUPnP").toBool());
Copy link
Contributor

@ryanofsky ryanofsky Jan 19, 2021

Choose a reason for hiding this comment

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

In commit "gui: Apply port mapping changes on dialog exit" (58e8364)

@hebasto It seems like this commit has a bug and will incorrectly overwrite command line and config file -upnp and -natpmp options. I think this might have happened before, but previously only if these settings were changed, now if the dialog is just opened and closed.

Would you have an opinion on reverting this change? Aside from the bug, it seems simpler to go back to fully applying settings in OptionsModel, instead of using half-using OptionsModel to apply the setting, and then reading the setting later in a different code path and QSettings instance in dialog code outside of OptionsModel.

(Encountered this while rebasing #15936)

Copy link
Member Author

@hebasto hebasto Jan 30, 2021

Choose a reason for hiding this comment

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

@ryanofsky

You are right.

It seems like this commit has a bug and will incorrectly overwrite command line and config file -upnp and -natpmp options.

The concerns about user-friendly and correct behavior were raised in #18184. Now users are able to switch -upnp and -natpmp on/off on-the-fly via the GUI. Should we drop this feature?

I think this might have happened before, but previously only if these settings were changed, now if the dialog is just opened and closed.

Exiting the dialog via the "Cancel" push button or pressing the "Esc" key should not trigger that code path.

Anyway, I'll happy to revert (or enhance) that change in a proper way.

random-zebra added a commit to PIVX-Project/PIVX that referenced this issue Jun 24, 2021
c198797 build: compile libnatpmp with -DNATPMP_STATICLIB on Windows (Fuzzbawls)
f2b62a3 build: use newer source for libnatpmp (fanquake)
8143139 GUI: Re-work port mapping saving (Fuzzbawls)
c4300c0 Clang-Tidy up mapport.cpp (Fuzzbawls)
f1951b5 Add libnatpmp to nightly snapcraft builds (Fuzzbawls)
d48ef00 doc: Add release notes (Fuzzbawls)
7886374 doc: Add libnatpmp stuff (Fuzzbawls)
cf992d0 ci: Add libnatpmp-dev package to some builds (Fuzzbawls)
5a5e3cf gui: Add NAT-PMP network option (Fuzzbawls)
9a4ba48 net: Add -natpmp command line option (Fuzzbawls)
9927296 net: Add NAT-PMP to port mapping loop (Hennadii Stepanov)
84cd118 net: Add initial libnatpmp support (Fuzzbawls)
faed148 gui: Apply port mapping changes on save (Fuzzbawls)
266d322 scripted-diff: Rename UPnP stuff (Fuzzbawls)
06dc0dd net: Add flags for port mapping protocols (Fuzzbawls)
2abea67 net: Keep trying to use UPnP when -upnp=1 (Hennadii Stepanov)
d2fa5c4 Make ThreadMapPort static (Fuzzbawls)
7532090 refactor: Replace magic number with named constant (Hennadii Stepanov)
c481f62 refactor: Move port mapping code to its own module (Fuzzbawls)

Pull request description:

  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.

  - [x] #2330 to be merged first.

ACKs for top commit:
  furszy:
    upnp and code check ACK c198797.
  random-zebra:
    utACK c198797 after rebase, and merging...

Tree-SHA512: e4b47dcd9589a1fd7a1bf3254c319d80af92c878e1657a17a169e49aa888aea33921a9428da3e8d6ee023d971143cf4a495e5a32957370174cff57e6cc6f2cc0
apoelstra pushed a commit to apoelstra/elements that referenced this issue Jul 11, 2021
Details: Homebrew/discussions#691

bitcoin/bitcoin#21663 (1/1)

Conflicts: .cirrus.yml; had to remove libnatpnp which was added upstream
 in bitcoin/bitcoin#18077 which is a much larger
 PR that we are NOT backporting.
apoelstra pushed a commit to apoelstra/elements that referenced this issue Jul 11, 2021
Details: Homebrew/discussions#691

bitcoin/bitcoin#21663 (1/1)

Conflicts: .cirrus.yml; had to remove libnatpnp which was added upstream
 in bitcoin/bitcoin#18077 which is a much larger
 PR that we are NOT backporting.
apoelstra pushed a commit to apoelstra/elements that referenced this issue Jul 11, 2021
Details: Homebrew/discussions#691

bitcoin/bitcoin#21663 (1/1)

Conflicts: .cirrus.yml; had to remove libnatpnp which was added upstream
 in bitcoin/bitcoin#18077 which is a much larger
 PR that we are NOT backporting.
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Feb 26, 2022
PastaPastaPasta added a commit to dashpay/dash that referenced this issue Mar 5, 2022
merge bitcoin#18077...22397: Add NAT-PMP port forwarding support
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue Apr 19, 2022
This also effectively reverts 58e8364 from
bitcoin#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue Apr 23, 2022
This also effectively reverts 58e8364 from
bitcoin#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 2, 2022
This also effectively reverts 58e8364 from
bitcoin#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 16, 2022
This also effectively reverts 58e8364 from
bitcoin#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 17, 2022
This also effectively reverts 58e8364 from
bitcoin#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 19, 2022
This also effectively reverts 58e8364 from
bitcoin#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue May 19, 2022
This also effectively reverts 58e8364 from
bitcoin#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 19, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 20, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 20, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 20, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 20, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 23, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 23, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 26, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 26, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue May 31, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue Jun 6, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
ryanofsky added a commit to ryanofsky/gui that referenced this issue Jun 9, 2022
This also effectively reverts 58e8364 from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
bitcoin/bitcoin#18077 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment