Repository: bitcoin/bitcoin (language: cpp)

Issue:
Check usages of `#if defined(...)`
It would seem that in most cases, we want `#if ...` instead of `#if defined(...)`, so let's make sure that when we say `#if defined(...)`, we actually mean it.

Inspiration: https://github.com/bitcoin/bitcoin/pull/16344


Discussion / hints:
I have gone through `bitcoin_config.h` and found a few of these.
Looks like https://github.com/bitcoin/bitcoin/pull/16547#issuecomment-522735491 is up for grabs?
I'm going to revive #16547 and look at turning on `-Wundef`.
I looked over the uses in 8be652e43964329c1f2dadd676916b53e5c40974 and found one bug: LevelDB is using `#if defined(HAVE_O_CLOEXEC)`, which we are defining to `0` on systems without it - this leads to a build failure, and should be fixed.
> I looked over the uses in [8be652e](https://github.com/bitcoin/bitcoin/commit/8be652e43964329c1f2dadd676916b53e5c40974) and found one bug: LevelDB is using `#if defined(HAVE_O_CLOEXEC)`, which we are defining to `0` on systems without it - this leads to a build failure, and should be fixed.

Fixed in #25739.
Is this issue up for grab?
> Is this issue up for grab?

It is. See #16547.
> It is. See https://github.com/bitcoin/bitcoin/pull/16547.

It isn't. See #25302.
On the master branch @ 4e8a7654f623fc0dad933b3d93dc54d8206c605d plus https://github.com/bitcoin/bitcoin/pull/27696, there are only two cases in our build system of explicit unconditional usage of the `AC_DEFINE` and `AC_DEFINE_UNQUOTED` macros (except for one which define string literals like `COPYRIGHT_YEAR`):

- https://github.com/bitcoin/bitcoin/blob/4e8a7654f623fc0dad933b3d93dc54d8206c605d/configure.ac#L1230
- https://github.com/bitcoin/bitcoin/blob/4e8a7654f623fc0dad933b3d93dc54d8206c605d/configure.ac#L1245

That means that the `HAVE_FDATASYNC` and `HAVE_O_CLOEXEC` are always defined, and them with `#if defined(...)`  is meaningless. All our code base uses `#if ...` for them.

 _All_ other use cases of the `AC_DEFINE` and `AC_DEFINE_UNQUOTED` macros are _conditional_ with explicitly provided value `1`. That means either a macro is defined and expands to `1` or it is not defined. Therefore, both `#if ...`  and `#if defined(...)` behave equally. However, we mean the latter.

Moreover, descriptions like "Define to 1 to enable wallet functions" should be replaced with "Define this symbol to enable wallet functions" as the actual value of the defined macro does not matter at all.

---

The bottom line:
> in most cases, we want `#if ...` instead of `#if defined(...)`, so let's make sure that when we say `#if defined(...)`, we actually mean it.

These cases are `HAVE_FDATASYNC` and `HAVE_O_CLOEXEC` only, which are OK now.

---

It is worth mentioning that macros like `AC_CHECK_DECLS` define always define symbols implicitly.

---

I don't think any changes in the source files are required. However, in Developer Notes, we can recommend to use `#if defined(...)` by default except for cases when  `#if ...` is required.

Fix patch:
diff --git a/configure.ac b/configure.ac
index ab369cc98afc7..23b8870d43407 100644
--- a/configure.ac
+++ b/configure.ac
@@ -405,6 +405,7 @@ AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough], [WARN_CXXFLAGS="$WARN_CXXFLAGS -
 AX_CHECK_COMPILE_FLAG([-Wunreachable-code], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code"], [], [$CXXFLAG_WERROR])
 AX_CHECK_COMPILE_FLAG([-Wdocumentation], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdocumentation"], [], [$CXXFLAG_WERROR])
 AX_CHECK_COMPILE_FLAG([-Wself-assign], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wself-assign"], [], [$CXXFLAG_WERROR])
+AX_CHECK_COMPILE_FLAG([-Wundef], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wundef"], [], [$CXXFLAG_WERROR])
 
 dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
 dnl unknown options if any other warning is produced. Test the -Wfoo case, and
@@ -1392,6 +1393,9 @@ if test "$with_libmultiprocess" = "yes" || test "$with_libmultiprocess" = "auto"
   PKG_CHECK_MODULES([LIBMULTIPROCESS], [libmultiprocess], [
      libmultiprocess_found=yes;
      libmultiprocess_prefix=`$PKG_CONFIG --variable=prefix libmultiprocess`;
+     if test "$suppress_external_warnings" != "no" ; then
+       LIBMULTIPROCESS_CFLAGS=SUPPRESS_WARNINGS($LIBMULTIPROCESS_CFLAGS)
+     fi
   ], [true])
 elif test "$with_libmultiprocess" != "no"; then
   AC_MSG_ERROR([--with-libmultiprocess=$with_libmultiprocess value is not yes, auto, or no])
diff --git a/src/Makefile.am b/src/Makefile.am
index 4a1973aa87620..fa1612b5b840f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1088,7 +1088,7 @@ libbitcoin_ipc_a_SOURCES = \
   ipc/process.cpp \
   ipc/process.h \
   ipc/protocol.h
-libbitcoin_ipc_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
+libbitcoin_ipc_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
 libbitcoin_ipc_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) $(LIBMULTIPROCESS_CFLAGS)
 
 include $(MPGEN_PREFIX)/include/mpgen.mk
diff --git a/src/init.cpp b/src/init.cpp
index 5bb82dc320945..16cfa296ccc77 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -109,7 +109,7 @@
 
 #include <boost/signals2/signal.hpp>
 
-#if ENABLE_ZMQ
+#ifdef ENABLE_ZMQ
 #include <zmq/zmqabstractnotifier.h>
 #include <zmq/zmqnotificationinterface.h>
 #include <zmq/zmqrpc.h>
@@ -364,7 +364,7 @@ void Shutdown(NodeContext& node)
         client->stop();
     }
 
-#if ENABLE_ZMQ
+#ifdef ENABLE_ZMQ
     if (g_zmq_notification_interface) {
         if (node.validation_signals) node.validation_signals->UnregisterValidationInterface(g_zmq_notification_interface.get());
         g_zmq_notification_interface.reset();
@@ -531,7 +531,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection memory usage for the send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target per 24h. Limit does not apply to peers with 'download' permission or blocks created within past week. 0 = no limit (default: %s). Optional suffix units [k|K|m|M|g|G|t|T] (default: M). Lowercase is 1000 base while uppercase is 1024 base", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-#if HAVE_SOCKADDR_UN
+#ifdef HAVE_SOCKADDR_UN
     argsman.AddArg("-onion=<ip:port|path>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy). May be a local file path prefixed with 'unix:'.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 #else
     argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -544,7 +544,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
-#if HAVE_SOCKADDR_UN
+#ifdef HAVE_SOCKADDR_UN
     argsman.AddArg("-proxy=<ip:port|path>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled). May be a local file path prefixed with 'unix:' if the proxy supports it.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_ELISION, OptionsCategory::CONNECTION);
 #else
     argsman.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_ELISION, OptionsCategory::CONNECTION);
@@ -578,7 +578,7 @@ void SetupServerArgs(ArgsManager& argsman)
 
     g_wallet_init_interface.AddWalletOptions(argsman);
 
-#if ENABLE_ZMQ
+#ifdef ENABLE_ZMQ
     argsman.AddArg("-zmqpubhashblock=<address>", "Enable publish hash block in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
     argsman.AddArg("-zmqpubhashtx=<address>", "Enable publish hash transaction in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
     argsman.AddArg("-zmqpubrawblock=<address>", "Enable publish raw block in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
@@ -1200,7 +1200,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     for (const auto& client : node.chain_clients) {
         client->registerRpcs();
     }
-#if ENABLE_ZMQ
+#ifdef ENABLE_ZMQ
     RegisterZMQRPCCommands(tableRPC);
 #endif
 
@@ -1325,7 +1325,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             std::string host_out;
             uint16_t port_out{0};
             if (!SplitHostPort(socket_addr, port_out, host_out)) {
-#if HAVE_SOCKADDR_UN
+#ifdef HAVE_SOCKADDR_UN
                 // Allow unix domain sockets for some options e.g. unix:/some/file/path
                 if (!unix || socket_addr.find(ADDR_PREFIX_UNIX) != 0) {
                     return InitError(InvalidPortErrMsg(arg, socket_addr));
@@ -1472,7 +1472,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             return InitError(ResolveErrMsg("externalip", strAddr));
     }
 
-#if ENABLE_ZMQ
+#ifdef ENABLE_ZMQ
     g_zmq_notification_interface = CZMQNotificationInterface::Create(
         [&chainman = node.chainman](std::vector<uint8_t>& block, const CBlockIndex& index) {
             assert(chainman);
diff --git a/src/netbase.cpp b/src/netbase.cpp
index fcbdb43e2a970..f5f0997ba654e 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -23,7 +23,7 @@
 #include <limits>
 #include <memory>
 
-#if HAVE_SOCKADDR_UN
+#ifdef HAVE_SOCKADDR_UN
 #include <sys/un.h>
 #endif
 
@@ -218,7 +218,7 @@ CService LookupNumeric(const std::string& name, uint16_t portDefault, DNSLookupF
 
 bool IsUnixSocketPath(const std::string& name)
 {
-#if HAVE_SOCKADDR_UN
+#ifdef HAVE_SOCKADDR_UN
     if (name.find(ADDR_PREFIX_UNIX) != 0) return false;
 
     // Split off "unix:" prefix
@@ -527,7 +527,7 @@ std::unique_ptr<Sock> CreateSockOS(int domain, int type, int protocol)
         return nullptr;
     }
 
-#if HAVE_SOCKADDR_UN
+#ifdef HAVE_SOCKADDR_UN
     if (domain == AF_UNIX) return sock;
 #endif
 
@@ -638,7 +638,7 @@ std::unique_ptr<Sock> Proxy::Connect() const
 
     if (!m_is_unix_socket) return ConnectDirectly(proxy, /*manual_connection=*/true);
 
-#if HAVE_SOCKADDR_UN
+#ifdef HAVE_SOCKADDR_UN
     auto sock = CreateSock(AF_UNIX, SOCK_STREAM, 0);
     if (!sock) {
         LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Cannot create a socket for connecting to %s\n", m_unix_socket_path);
diff --git a/src/randomenv.cpp b/src/randomenv.cpp
index aeec959c28c61..49033deef267f 100644
--- a/src/randomenv.cpp
+++ b/src/randomenv.cpp
@@ -42,15 +42,15 @@
 #if HAVE_DECL_GETIFADDRS && HAVE_DECL_FREEIFADDRS
 #include <ifaddrs.h>
 #endif
-#if HAVE_SYSCTL
+#ifdef HAVE_SYSCTL
 #include <sys/sysctl.h>
-#if HAVE_VM_VM_PARAM_H
+#ifdef HAVE_VM_VM_PARAM_H
 #include <vm/vm_param.h>
 #endif
-#if HAVE_SYS_RESOURCES_H
+#ifdef HAVE_SYS_RESOURCES_H
 #include <sys/resources.h>
 #endif
-#if HAVE_SYS_VMMETER_H
+#ifdef HAVE_SYS_VMMETER_H
 #include <sys/vmmeter.h>
 #endif
 #endif
@@ -162,7 +162,7 @@ void AddPath(CSHA512& hasher, const char *path)
 }
 #endif
 
-#if HAVE_SYSCTL
+#ifdef HAVE_SYSCTL
 template<int... S>
 void AddSysctl(CSHA512& hasher)
 {
@@ -274,7 +274,7 @@ void RandAddDynamicEnv(CSHA512& hasher)
     AddFile(hasher, "/proc/self/status");
 #endif
 
-#if HAVE_SYSCTL
+#ifdef HAVE_SYSCTL
 #  ifdef CTL_KERN
 #    if defined(KERN_PROC) && defined(KERN_PROC_ALL)
     AddSysctl<CTL_KERN, KERN_PROC, KERN_PROC_ALL>(hasher);
@@ -419,7 +419,7 @@ void RandAddStaticEnv(CSHA512& hasher)
 
     // For MacOS/BSDs, gather data through sysctl instead of /proc. Not all of these
     // will exist on every system.
-#if HAVE_SYSCTL
+#ifdef HAVE_SYSCTL
 #  ifdef CTL_HW
 #    ifdef HW_MACHINE
     AddSysctl<CTL_HW, HW_MACHINE>(hasher);


Test patch: