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

fanquake commented at 2:09 PM on January 16, 2023: member

This PR removes the --enable-upnp-default and --enable-natpmp-default options from configure.

It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state (but no other similar options), and I'm not sure what use-case it satisfies, that can't be achieved by multiple other means. I also doubt that we'll ever restart using these in release builds, or turning on any of this by default.

I think the only scenario these options would be used is when you want to compile your own binaries (we don't use them in Guix), with port-forwarding on by default, but otherwise can't or don't want to use a .conf file, can't or don't want to pass command line options at runtime, and also don't want to modify the source code?

fanquake added the label Build system on Jan 16, 2023

DrahtBot commented at 2:10 PM on January 16, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

fanquake force-pushed on Jan 16, 2023

fanquake added the label DrahtBot Guix build requested on Jan 16, 2023

fanquake force-pushed on Jan 16, 2023

fanquake marked this as a draft on Jan 16, 2023

fanquake force-pushed on Jan 16, 2023

fanquake marked this as ready for review on Jan 16, 2023

DrahtBot commented at 10:11 AM on January 17, 2023: contributor

Guix builds

File	commit 04e54fd21fdf7374fb28212cee3efed3da6ea220<br>(master)	commit ad58437cd9d0c75672fda816b20c98d5c473d41d<br>(master and this pull)
SHA256SUMS.part	`4e1d0230577de4b6...`	`1510f7f67a92de28...`
*-aarch64-linux-gnu-debug.tar.gz	`37b3dd585a67552b...`	`d44baf9514c2a408...`
*-aarch64-linux-gnu.tar.gz	`f8a39a712a54b903...`	`1ba4d5982b4809a8...`
*-arm-linux-gnueabihf-debug.tar.gz	`90ea67ece79851fd...`	`65bd548f2d4fe116...`
*-arm-linux-gnueabihf.tar.gz	`40e33f85df7d9302...`	`8dbb4451a4587cc8...`
*-arm64-apple-darwin-unsigned.dmg	`a4023e327f78e7a6...`	`151d71042e972435...`
*-arm64-apple-darwin-unsigned.tar.gz	`13b2eb996655bef7...`	`7d659e40083fff1c...`
*-arm64-apple-darwin.tar.gz	`8b9155333f54be43...`	`afef25289fcfb088...`
*-powerpc64-linux-gnu-debug.tar.gz	`75781b21d583604a...`	`d0d9eacb278e8e25...`
*-powerpc64-linux-gnu.tar.gz	`7ea8b7d8c56b892a...`	`caed1d1c0c6af274...`
*-powerpc64le-linux-gnu-debug.tar.gz	`d5ed38dd3278db24...`	`378cf3499cef1e74...`
*-powerpc64le-linux-gnu.tar.gz	`e78071c841a39641...`	`a4a124357540e700...`
*-riscv64-linux-gnu-debug.tar.gz	`c39095ce79144fdb...`	`d0a571de48b80aaf...`
*-riscv64-linux-gnu.tar.gz	`1f5ffb6ac44c0e65...`	`95c01f5f9be67263...`
*-win64-debug.zip	`2693ef1f1e45a6d8...`	`84778fa05565046f...`
*-win64-setup-unsigned.exe	`62c1bf0fe0d4544c...`	`86d3c24f5e224108...`
*-win64-unsigned.tar.gz	`7b591622ff2b9320...`	`94f8a5f5b4fd4a80...`
*-win64.zip	`84f6b8f44f16ecc7...`	`7f5e1fe52439567f...`
*-x86_64-apple-darwin-unsigned.dmg	`dc7b045515b73030...`	`44da22a79a1c6c8b...`
*-x86_64-apple-darwin-unsigned.tar.gz	`d49d68f4cc848714...`	`1dfc51de8c3b44e8...`
*-x86_64-apple-darwin.tar.gz	`838793c521fec733...`	`e992e2147c762799...`
*-x86_64-linux-gnu-debug.tar.gz	`dcdee05e7963e4be...`	`0d811372445b09d0...`
*-x86_64-linux-gnu.tar.gz	`994d6a153465aa8d...`	`fbde669fa4502e74...`
*.tar.gz	`cc701c23fc62efbf...`	`9a0120f431b28405...`
guix_build.log	`3da50f15ac7713d2...`	`6fe4d7d6cfe8d61d...`
guix_build.log.diff		`32304bf0d3dc6c42...`

DrahtBot removed the label DrahtBot Guix build requested on Jan 17, 2023

TheCharlatan commented at 9:12 PM on January 20, 2023: contributor

Concept ACK

Looking at the history, the --enable-upnp-default has been around since the switch to autotools. Before that, when the project was still using qmake, it was possible to pass qmake "USE_UPNP=1 to achieve the same behavior. Similar options were available with qmake "USE_DBUS=1 and qmake "USE_QRCODE" (though for one reason or another these were just translated into a single with-* configure option) and qmake "USE_IPV6", which was translated to --enable-ipv6. The npmp behavior seems to have just copied the existing upnp checks.

Dbus and qrencode are now handled in "special" qt configure checks, while the ipv6 option was removed in #4115. Control of ipv6 usage was still somewhat retained through the onlynet argument. Similar arguments apply to this case as well.

hebasto commented at 3:50 PM on January 25, 2023: member

Concept ACK on:

It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state

The npmp behavior seems to have just copied the existing upnp checks.

That's true.

in configure.ac:1757 in fb8f8474c4 outdated

1760 | -      use_upnp_default=yes
1761 | -      upnp_setting=1
1762 | -    fi
1763 | -    AC_MSG_RESULT([$use_upnp_default])
1764 | -    AC_DEFINE_UNQUOTED([USE_UPNP],[$upnp_setting],[UPnP support not compiled if undefined, otherwise value (0 or 1) determines default state])
1765 | +    AC_DEFINE_UNQUOTED([USE_UPNP], [1], [UPnP support])

hebasto commented at 12:03 PM on January 28, 2023:

    AC_DEFINE([USE_UPNP], [1], [Define to 1 if UPnP support should be compiled in.])

fanquake commented at 3:31 PM on January 28, 2023:

Done.

in configure.ac:1778 in fb8f8474c4 outdated

1781 | -      use_natpmp_default=yes
1782 | -      natpmp_setting=1
1783 | -    fi
1784 | -    AC_MSG_RESULT($use_natpmp_default)
1785 | -    AC_DEFINE_UNQUOTED([USE_NATPMP], [$natpmp_setting], [NAT-PMP support not compiled if undefined, otherwise value (0 or 1) determines default state])
1786 | +    AC_DEFINE_UNQUOTED([USE_NATPMP], [1], [NAT-PMP support])

hebasto commented at 12:04 PM on January 28, 2023:

    AC_DEFINE([USE_NATPMP], [1], [Define to 1 if NAT-PMP support should be compiled in.])

fanquake commented at 3:31 PM on January 28, 2023:

Done.

hebasto approved

hebasto commented at 12:20 PM on January 28, 2023: member

ACK fb8f8474c41e4de4ce08e6476d6f4399a687d2c0

The content of src/config/bitcoin-config.h:

on master (6b7ccb98a59fa6f702b6dda007f003cffee19009)

$ grep -B 2 -E USE_UPNP\|USE_NATPMP src/config/bitcoin-config.h
/* NAT-PMP support not compiled if undefined, otherwise value (0 or 1)
   determines default state */
#define USE_NATPMP 0
--
/* UPnP support not compiled if undefined, otherwise value (0 or 1) determines
   default state */
#define USE_UPNP 0

this PR:

$ grep -B 2 -E USE_UPNP\|USE_NATPMP src/config/bitcoin-config.h

/* NAT-PMP support */
#define USE_NATPMP 1
--

/* UPnP support */
#define USE_UPNP 1

Two suggestions only:

No need to use AC_DEFINE_UNQUOTED macros. The AC_DEFINE works just fine as no arguments require expansion.
Maybe document the value, which a macro is defined to, explicitly?

Remove configure-time setting of DEFAULT_NATPMP

Default to false.

06562e5fa7

Remove configure-time setting of DEFAULT_UPNP

Default to false.

25a0e8ba0b

build: remove --enable-natpmp-default from configure 02f5a5e7b5

build: remove --enable-upnp-default from configure 2b248798d9

doc: add release notes for 26896 d51f0fa4b7

fanquake force-pushed on Jan 28, 2023

hebasto approved

hebasto commented at 4:21 PM on January 28, 2023: member

ACK d51f0fa4b7b19281efe65aacf414845c661d0a13, rebased and comments have been addressed since my recent review.

TheCharlatan approved

TheCharlatan commented at 9:59 PM on January 29, 2023: contributor

ACK d51f0fa4b7b19281efe65aacf414845c661d0a13

fanquake merged this on Jan 30, 2023

fanquake closed this on Jan 30, 2023

fanquake deleted the branch on Jan 30, 2023

sidhujag referenced this in commit f11b3da265 on Jan 30, 2023

knst referenced this in commit 659068c1df on Nov 9, 2023

knst referenced this in commit f7b6dcc7ea on Nov 17, 2023

knst referenced this in commit c4710425b2 on Nov 17, 2023

knst referenced this in commit 437a5921cb on Nov 20, 2023

knst referenced this in commit de9a0171e6 on Nov 20, 2023

PastaPastaPasta referenced this in commit 26211cc22f on Dec 4, 2023

PastaPastaPasta referenced this in commit 63b907126f on Dec 4, 2023

bitcoin locked this on Jan 30, 2024