doc: fixup help output for -upnp and -natpmp #28874

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:redundant_upnp_ifdef changing 1 files +2 −6

fanquake commented at 4:49 PM on November 14, 2023: member

This is a very belated followup to #26896 (which removed the configure options for setting the upnp and natpmp runtime default) and corrects the -help docs for -upnp and -natpmp.
DrahtBot commented at 4:50 PM on November 14, 2023: contributor



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



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK davidgumberg, hernanmarino

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
fanquake renamed this:
~~Redundant upnp ifdef~~
doc: fixup help output for -upnp and -natpmp
on Nov 14, 2023
DrahtBot added the label Docs on Nov 14, 2023

Type	Reviewers
ACK	davidgumberg, hernanmarino

in src/init.cpp:522 in 372cd23f64 outdated

 518 | @@ -519,16 +519,12 @@ void SetupServerArgs(ArgsManager& argsman)
 519 |      argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port of %i will be used.", DEFAULT_TOR_CONTROL, DEFAULT_TOR_CONTROL_PORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 520 |      argsman.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION);
 521 |  #ifdef USE_UPNP
 522 | -#if USE_UPNP
 523 | -    argsman.AddArg("-upnp", "Use UPnP to map the listening port (default: 1 when listening and no -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 524 | -#else
 525 | -    argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %u)", 0), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 526 | -#endif
 527 | +    argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %s)", DEFAULT_UPNP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

luke-jr commented at 11:44 PM on November 15, 2023:

    argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %s)", !Assert(!DEFAULT_UPNP)), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

Otherwise it would be incorrect (missing "when listening and no -proxy") if DEFAULT_UPNP flips to true.

Alternatively, just remove the SoftSetArg stuff...

fanquake commented at 9:30 AM on November 16, 2023:

if DEFAULT_UPNP flips to true.

I very much doubt we would ever return to shipping upnp/natpmp on by default.

luke-jr commented at 6:59 PM on November 24, 2023:

Hence why it might make more sense to just remove the SoftSetArg stuff that gives it a conditional default in that case

init: remove redundant upnp #ifdef 02395edca9

doc: fixup NAT-PMP help doc

This always defaults to false, since we removed the compile time options
to set it otherwise.

92f88a9629

in src/init.cpp:527 in 372cd23f64 outdated

 528 |  #else
 529 |      hidden_args.emplace_back("-upnp");
 530 |  #endif
 531 |  #ifdef USE_NATPMP
 532 | -    argsman.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %s)", DEFAULT_NATPMP ? "1 when listening and no -proxy" : "0"), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 533 | +    argsman.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %s)", DEFAULT_NATPMP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

luke-jr commented at 1:03 AM on November 16, 2023:

%s will make DEFAULT_NATPMP "false" here instead of "0", I think...

fanquake force-pushed on Nov 16, 2023
DrahtBot added the label CI failed on Jan 16, 2024
davidgumberg commented at 12:53 PM on April 9, 2024: contributor
ACK https://github.com/bitcoin/bitcoin/commit/92f88a962908c49dde99c03a4608e63e4a6eec71

Configured --with-natpmp and verified the help messages.
```
$ bitcoind --help

[...]
  -natpmp
       Use NAT-PMP to map the listening port (default: 0)
[...]
  -upnp
       Use UPnP to map the listening port (default: 0)
```
Verified that modifying bool DEFAULT_UPNP and bool DEFAULT_NATPMP changes the --help output.
hernanmarino commented at 11:11 PM on April 12, 2024: contributor

ACK 92f88a962908c49dde99c03a4608e63e4a6eec71
fanquake merged this on Apr 15, 2024
fanquake closed this on Apr 15, 2024
fanquake deleted the branch on Apr 15, 2024
bitcoin locked this on Apr 15, 2025

Contributors

Labels

Linked (view graph)

#26896 build: Remove port-forwarding runtime setting options from configure

doc: fixup help output for -upnp and -natpmp #28874

Code Coverage

Reviews