Merge #6559: backport: merge bitcoin#22903, bitcoin#24753, bitcoin#24831

, bitcoin#28240, bitcoin#27012, bitcoin#25713, bitcoin#24971, bitcoin#25047 (clang-tidy and IWYU)

4009777 refactor: apply IWYU to `rpc/{fees,signmessage}.cpp` (Kittywhiskers Van Gogh)
85d96c2 merge bitcoin#25047: add readability-redundant-declaration (Kittywhiskers Van Gogh)
71b400d merge bitcoin#24971: modernize-use-nullptr (Kittywhiskers Van Gogh)
104ba87 merge bitcoin#25713: run clang-tidy in quiet mode (Kittywhiskers Van Gogh)
5647652 merge bitcoin#27012: Print iwyu patch in git diff format (Kittywhiskers Van Gogh)
2980992 merge bitcoin#28240: Remove unused boost signals2 from torcontrol (Kittywhiskers Van Gogh)
8e8faa2 merge bitcoin#24831: add include-what-you-use (Kittywhiskers Van Gogh)
c673048 merge bitcoin#24753: Add clang-tidy task (Kittywhiskers Van Gogh)
a6e80ec merge bitcoin#22903: Enable clang-tidy bugprone-argument-comment and fix violations (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * While linting is generally reserved for our `arm_linux` build variant, utilizing IWYU (which in turns relies on `bear` for the compilation database, also relied upon by `clang-tidy`) requires a version paired to the Clang version ([source](https://github.com/dashpay/dash/blob/0b42caa2e84636e9a2901b485ea87627014a013b/contrib/containers/ci/Dockerfile#L139)) used to build the target codebase.

    This means it has to be run on a build variant that uses Clang. We have opted to use `linux64_multiprocess` as it the most lightweight Clang build variant available (compared to fuzz, UBSan and TSan variants which come with sanitizers tacked on).

  * An earlier version of this PR ([source](https://github.com/dashpay/dash/tree/2676eb914d7a1ca6a0f194fe5c573294c3ebee2f)) attempted to apply IWYU to Dash-specific code. This attempt was aborted because IWYU was found to behave non-deterministically.

    Furthermore, while IWYU will tell you what headers to use, it will not do the following:
    * Respect the usage of angle brackets for source headers (it will almost always suggest quotation marks)
    * Use C++-style headers (it will prefer `string.h` over `cstring` and changing it is a manual errand)
    * Apply its suggestions in alphabetical order (they will pool up at the bottom)
    * Respect newlines meant to separate sets of headers
    * Remain consistent (even if IWYU passes locally, there is no guarantee that it would translate to a green CI run)

    But it will do the following:
    * Change its suggestions based on the ordering of headers
    * Make suggestions to add headers/forward declarations and then when run a second time, urge you to remove them
    * Pull in implementation headers that shouldn't be directly included. IWYU currently ships with mappings to avoid this for the standard library ([source](https://github.com/include-what-you-use/include-what-you-use/blob/f88f045131bfa0ac709d1e0c103580666254b2c6/gcc.libc.imp)), Qt ([source](https://github.com/include-what-you-use/include-what-you-use/blob/f88f045131bfa0ac709d1e0c103580666254b2c6/qt5_11.imp)) and Boost ([source](https://github.com/include-what-you-use/include-what-you-use/blob/f88f045131bfa0ac709d1e0c103580666254b2c6/boost-all.imp)) but for all else, you're on your own.

    It was determined for now that we will simply take guidance from upstream on the matter and extend it to Dash code at some point in the future.

  * Both [bitcoin#25029](bitcoin#25029) and [bitcoin#25013](bitcoin#25013) made sure they pass IWYU, changes needed to ensure that the linter is satisfied were made in this PR.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 4009777
  PastaPastaPasta:
    utACK 4009777

Tree-SHA512: 0dae87abfd1f309300a3caadff565da831122591611cb0f6a7dc620e3edc775aeef954935b19035e2fd5b8334bd2281fb05b193a3bdb0844cd0f03890f506212

ci/dash/build_src.sh

@@ -51,7 +51,12 @@ make distdir VERSION=$BUILD_TARGET
 cd dashcore-$BUILD_TARGET
 bash -c "./configure $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG" || ( cat config.log && false)
 
-make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && make $GOAL V=1 ; false )
+if [ "${RUN_TIDY}" = "true" ]; then
+  MAYBE_BEAR="bear --config src/.bear-tidy-config"
+  MAYBE_TOKEN="--"
+fi
+
+bash -c "${MAYBE_BEAR} ${MAYBE_TOKEN} make ${MAKEJOBS} ${GOAL}" || ( echo "Build failure. Verbose build follows." && make $GOAL V=1 ; false )
 
 ccache --version | head -n 1 && ccache --show-stats
 
@@ -60,6 +65,24 @@ if [ -n "$USE_VALGRIND" ]; then
     ${BASE_ROOT_DIR}/ci/test/wrap-valgrind.sh
 fi
 
+if [ "${RUN_TIDY}" = "true" ]; then
+  set -eo pipefail
+  cd src
+  ( run-clang-tidy -quiet "${MAKEJOBS}" ) | grep -C5 "error"
+  cd ..
+  iwyu_tool.py \
+    "src/compat" \
+    "src/init" \
+    "src/rpc/fees.cpp" \
+    "src/rpc/signmessage.cpp" \
+    -p . "${MAKEJOBS}" \
+    -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
+    |& tee "/tmp/iwyu_ci.out"
+  cd src
+  fix_includes.py --nosafe_headers < /tmp/iwyu_ci.out
+  git --no-pager diff
+fi
+
 if [ "$RUN_SECURITY_TESTS" = "true" ]; then
   make test-security-check
 fi

ci/test/00_setup_env.sh

@@ -38,6 +38,7 @@ export USE_BUSY_BOX=${USE_BUSY_BOX:-false}
 
 export RUN_UNIT_TESTS=${RUN_UNIT_TESTS:-true}
 export RUN_FUNCTIONAL_TESTS=${RUN_FUNCTIONAL_TESTS:-true}
+export RUN_TIDY=${RUN_TIDY:-false}
 export RUN_SECURITY_TESTS=${RUN_SECURITY_TESTS:-false}
 # By how much to scale the test_runner timeouts (option --timeout-factor).
 # This is needed because some ci machines have slow CPU or disk, so sanitizers

ci/test/00_setup_env_native_multiprocess.sh

@@ -10,7 +10,10 @@ export CONTAINER_NAME=ci_native_multiprocess
 export HOST=x86_64-pc-linux-gnu
 export PACKAGES="cmake python3 llvm clang"
 export DEP_OPTS="MULTIPROCESS=1 CC=clang-18 CXX=clang++-18"
+export RUN_TIDY=true
 export GOAL="install"
 export TEST_RUNNER_EXTRA="--v2transport"
 export BITCOIN_CONFIG="--with-boost-process --enable-debug CC=clang-18 CXX=clang++-18" # Use clang to avoid OOM
+# Additional flags for RUN_TIDY
+export BITCOIN_CONFIG="${BITCOIN_CONFIG} --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0'"
 export BITCOIND=dash-node  # Used in functional tests

configure.ac

@@ -1911,6 +1911,8 @@ AC_CONFIG_LINKS([contrib/devtools/test-security-check.py:contrib/devtools/test-s
 AC_CONFIG_LINKS([contrib/devtools/symbol-check.py:contrib/devtools/symbol-check.py])
 AC_CONFIG_LINKS([contrib/devtools/test-symbol-check.py:contrib/devtools/test-symbol-check.py])
 AC_CONFIG_LINKS([contrib/filter-lcov.py:contrib/filter-lcov.py])
+AC_CONFIG_LINKS([src/.bear-tidy-config:src/.bear-tidy-config])
+AC_CONFIG_LINKS([src/.clang-tidy:src/.clang-tidy])
 AC_CONFIG_LINKS([test/functional/test_runner.py:test/functional/test_runner.py])
 AC_CONFIG_LINKS([test/fuzz/test_runner.py:test/fuzz/test_runner.py])
 AC_CONFIG_LINKS([test/util/test_runner.py:test/util/test_runner.py])

contrib/containers/ci/Dockerfile

@@ -39,6 +39,7 @@ RUN set -ex; \
     autotools-dev \
     automake \
     autoconf \
+    bear \
     bison \
     build-essential \
     bsdmainutils \
@@ -83,12 +84,14 @@ RUN set -ex; \
     "clang-tidy-${LLVM_VERSION}" \
     "libc++-${LLVM_VERSION}-dev" \
     "libc++abi-${LLVM_VERSION}-dev" \
+    "libclang-${LLVM_VERSION}-dev" \
     "libclang-rt-${LLVM_VERSION}-dev" \
-    "lld-${LLVM_VERSION}"; \
+    "lld-${LLVM_VERSION}" \
+    "llvm-${LLVM_VERSION}-dev"; \
     rm -rf /var/lib/apt/lists/*; \
     echo "Setting defaults..."; \
     lldbUpdAltArgs="update-alternatives --install /usr/bin/llvm-config llvm-config /usr/bin/llvm-config-${LLVM_VERSION} 100"; \
-    for binName in clang clang++ clang-format clang-tidy clangd dsymutil lld lldb lldb-server llvm-ar llvm-cov llvm-nm llvm-objdump llvm-ranlib llvm-strip; do \
+    for binName in clang clang++ clang-apply-replacements clang-format clang-tidy clangd dsymutil lld lldb lldb-server llvm-ar llvm-cov llvm-nm llvm-objdump llvm-ranlib llvm-strip run-clang-tidy; do \
         lldbUpdAltArgs="${lldbUpdAltArgs} --slave /usr/bin/${binName} ${binName} /usr/bin/${binName}-${LLVM_VERSION}"; \
     done; \
     for binName in ld64.lld ld.lld lld-link wasm-ld; do \
@@ -140,6 +143,14 @@ RUN set -ex; \
     cd dash_hash && pip3 install -r requirements.txt .; \
     cd .. && rm -rf dash_hash
 
+RUN set -ex; \
+    git clone --depth=1 "https://github.com/include-what-you-use/include-what-you-use" -b "clang_${LLVM_VERSION}" /opt/iwyu; \
+    cd /opt/iwyu; \
+    mkdir build && cd build; \
+    cmake -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-${LLVM_VERSION} ..; \
+    make install -j "$(( $(nproc) - 1 ))"; \
+    cd /opt && rm -rf /opt/iwyu;
+
 ARG SHELLCHECK_VERSION=v0.7.1
 RUN set -ex; \
     curl -fL "https://github.com/koalaman/shellcheck/releases/download/${SHELLCHECK_VERSION}/shellcheck-${SHELLCHECK_VERSION}.linux.x86_64.tar.xz" -o /tmp/shellcheck.tar.xz; \

contrib/devtools/iwyu/bitcoin.core.imp

@@ -0,0 +1,6 @@
+# Fixups / upstreamed changes
+[
+  { include: [ "<bits/termios-c_lflag.h>", private, "<termios.h>", public ] },
+  { include: [ "<bits/termios-struct.h>", private, "<termios.h>", public ] },
+  { include: [ "<bits/termios-tcflow.h>", private, "<termios.h>", public ] },
+]

src/.bear-tidy-config

@@ -0,0 +1,23 @@
+{
+  "output": {
+    "content": {
+      "include_only_existing_source": true,
+      "paths_to_include": [],
+      "paths_to_exclude": [
+        "src/crc32",
+        "src/crypto/x11",
+        "src/dashbls",
+        "src/gsl",
+        "src/immer",
+        "src/leveldb",
+        "src/minisketch",
+        "src/univalue",
+        "src/secp256k1"
+      ]
+    },
+    "format": {
+      "command_as_array": true,
+      "drop_output_field": false
+    }
+  }
+}

src/.clang-tidy

@@ -0,0 +1,11 @@
+Checks: '
+-*,
+bugprone-argument-comment,
+modernize-use-nullptr,
+readability-redundant-declaration,
+'
+WarningsAsErrors: '
+bugprone-argument-comment,
+modernize-use-nullptr,
+readability-redundant-declaration,
+'

src/bench/coin_selection.cpp

@@ -51,7 +51,7 @@ static void CoinSelection(benchmark::Bench& bench)
     const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34,
                                                     /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
                                                     /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
-                                                    /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false);
+                                                    /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
     bench.run([&] {
         std::set<CInputCoin> setCoinsRet;
         CAmount nValueRet;

src/compat/byteswap.h

@@ -9,7 +9,7 @@
 #include <config/bitcoin-config.h>
 #endif
 
-#include <stdint.h>
+#include <cstdint>
 
 #if defined(HAVE_BYTESWAP_H)
 #include <byteswap.h>

src/compat/compat.h

@@ -22,19 +22,18 @@
 #include <ws2tcpip.h>
 #include <cstdint>
 #else
-#include <fcntl.h>
-#include <sys/mman.h>
-#include <sys/select.h>
-#include <sys/socket.h>
-#include <sys/types.h>
-#include <net/if.h>
-#include <netinet/in.h>
-#include <netinet/tcp.h>
-#include <arpa/inet.h>
-#include <ifaddrs.h>
-#include <limits.h>
-#include <netdb.h>
-#include <unistd.h>
+#include <arpa/inet.h>   // IWYU pragma: export
+#include <fcntl.h>       // IWYU pragma: export
+#include <ifaddrs.h>     // IWYU pragma: export
+#include <net/if.h>      // IWYU pragma: export
+#include <netdb.h>       // IWYU pragma: export
+#include <netinet/in.h>  // IWYU pragma: export
+#include <netinet/tcp.h> // IWYU pragma: export
+#include <sys/mman.h>    // IWYU pragma: export
+#include <sys/select.h>  // IWYU pragma: export
+#include <sys/socket.h>  // IWYU pragma: export
+#include <sys/types.h>   // IWYU pragma: export
+#include <unistd.h>      // IWYU pragma: export
 #endif
 
 // We map Linux / BSD error functions and codes, to the equivalent

src/compat/cpuid.h

@@ -10,6 +10,8 @@
 
 #include <cpuid.h>
 
+#include <cstdint>
+
 // We can't use cpuid.h's __get_cpuid as it does not support subleafs.
 void static inline GetCPUID(uint32_t leaf, uint32_t subleaf, uint32_t& a, uint32_t& b, uint32_t& c, uint32_t& d)
 {

src/compat/endian.h

@@ -11,7 +11,7 @@
 
 #include <compat/byteswap.h>
 
-#include <stdint.h>
+#include <cstdint>
 
 #if defined(HAVE_ENDIAN_H)
 #include <endian.h>

src/compat/stdin.cpp

@@ -2,23 +2,19 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#if defined(HAVE_CONFIG_H)
-#include <config/bitcoin-config.h>
-#endif
+#include <compat/stdin.h>
 
-#include <cstdio>       // for fileno(), stdin
+#include <cstdio>
 
 #ifdef WIN32
-#include <windows.h>    // for SetStdinEcho()
-#include <io.h>         // for isatty()
+#include <windows.h>
+#include <io.h>
 #else
-#include <termios.h>    // for SetStdinEcho()
-#include <unistd.h>     // for SetStdinEcho(), isatty()
-#include <poll.h>       // for StdinReady()
+#include <termios.h>
+#include <unistd.h>
+#include <poll.h>
 #endif
 
-#include <compat/stdin.h>
-
 // https://stackoverflow.com/questions/1413445/reading-a-password-from-stdcin
 void SetStdinEcho(bool enable)
 {

src/governance/governance.cpp

@@ -1545,7 +1545,7 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co
 
 void CGovernanceManager::RequestOrphanObjects(CConnman& connman)
 {
-    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
+    const CConnman::NodesSnapshot snap{connman, /* cond = */ CConnman::FullyConnectedOnly};
 
     std::vector<uint256> vecHashesFiltered;
     {

src/init/common.cpp

@@ -9,16 +9,20 @@
 #include <bls/bls.h>
 #include <clientversion.h>
 #include <crypto/sha256.h>
+#include <fs.h>
 #include <key.h>
 #include <logging.h>
 #include <node/interface_ui.h>
 #include <random.h>
+#include <tinyformat.h>
 #include <util/string.h>
 #include <util/system.h>
 #include <util/time.h>
 #include <util/translation.h>
 
-#include <memory>
+#include <algorithm>
+#include <string>
+#include <vector>
 
 namespace init {
 void SetGlobals()

src/llmq/signing_shares.cpp

@@ -1113,7 +1113,7 @@ bool CSigSharesManager::SendMessages(CConnman& connman)
         return session->sendSessionId;
     };
 
-    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
+    const CConnman::NodesSnapshot snap{connman, /* cond = */ CConnman::FullyConnectedOnly};
     {
         LOCK(cs);
         CollectSigSharesToRequest(sigSharesToRequest);

src/mapport.cpp

@@ -195,7 +195,7 @@ static bool ProcessUpnp()
         std::string strDesc = PACKAGE_NAME " " + FormatFullVersion();
 
         do {
-            r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, port.c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", 0, "0");
+            r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, port.c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", nullptr, "0");
 
             if (r != UPNPCOMMAND_SUCCESS) {
                 ret = false;
@@ -208,7 +208,7 @@ static bool ProcessUpnp()
         } while (g_mapport_interrupt.sleep_for(PORT_MAPPING_REANNOUNCE_PERIOD));
         g_mapport_interrupt.reset();
 
-        r = UPNP_DeletePortMapping(urls.controlURL, data.first.servicetype, port.c_str(), "TCP", 0);
+        r = UPNP_DeletePortMapping(urls.controlURL, data.first.servicetype, port.c_str(), "TCP", nullptr);
         LogPrintf("UPNP_DeletePortMapping() returned: %d\n", r);
         freeUPNPDevlist(devlist); devlist = nullptr;
         FreeUPNPUrls(&urls);

src/masternode/sync.cpp

@@ -139,7 +139,7 @@ void CMasternodeSync::ProcessTick(const PeerManager& peerman, const CGovernanceM
     }
 
     nTimeLastProcess = GetTime();
-    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
+    const CConnman::NodesSnapshot snap{connman, /* cond = */ CConnman::FullyConnectedOnly};
 
     // gradually request the rest of the votes after sync finished
     if(IsSynced()) {

src/net.cpp

@@ -134,9 +134,6 @@ static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 500;
 
 const std::string NET_MESSAGE_TYPE_OTHER = "*other*";
 
-constexpr const CConnman::CFullyConnectedOnly CConnman::FullyConnectedOnly;
-constexpr const CConnman::CAllNodes CConnman::AllNodes;
-
 static const uint64_t RANDOMIZER_ID_NETGROUP = 0x6c0edd8036ef4036ULL; // SHA256("netgroup")[0:8]
 static const uint64_t RANDOMIZER_ID_LOCALHOSTNONCE = 0xd93e69e2bbfa5735ULL; // SHA256("localhostnonce")[0:8]
 static const uint64_t RANDOMIZER_ID_ADDRCACHE = 0x1cf2e4ddd306dda9ULL; // SHA256("addrcache")[0:8]
@@ -2331,7 +2328,7 @@ void CConnman::CalculateNumConnectionsChangedStats()
     }
     mapRecvBytesMsgStats[NET_MESSAGE_TYPE_OTHER] = 0;
     mapSentBytesMsgStats[NET_MESSAGE_TYPE_OTHER] = 0;
-    const NodesSnapshot snap{*this, /* filter = */ CConnman::FullyConnectedOnly};
+    const NodesSnapshot snap{*this, /* cond = */ CConnman::FullyConnectedOnly};
     for (auto pnode : snap.Nodes()) {
         WITH_LOCK(pnode->cs_vRecv, pnode->UpdateRecvMapWithStats(mapRecvBytesMsgStats));
         WITH_LOCK(pnode->cs_vSend, pnode->UpdateSentMapWithStats(mapSentBytesMsgStats));
@@ -2700,7 +2697,7 @@ void CConnman::SocketHandler(CMasternodeSync& mn_sync)
     }();
 
     {
-        const NodesSnapshot snap{*this, /* filter = */ CConnman::AllNodes, /* shuffle = */ false};
+        const NodesSnapshot snap{*this, /* cond = */ CConnman::AllNodes, /* shuffle = */ false};
 
         // Check for the readiness of the already connected sockets and the
         // listening sockets in one call ("readiness" as in poll(2) or
@@ -3972,7 +3969,7 @@ void CConnman::ThreadMessageHandler()
         // Randomize the order in which we process messages from/to our peers.
         // This prevents attacks in which an attacker exploits having multiple
         // consecutive connections in the m_nodes list.
-        const NodesSnapshot snap{*this, /* filter = */ CConnman::AllNodes, /* shuffle = */ true};
+        const NodesSnapshot snap{*this, /* cond = */ CConnman::AllNodes, /* shuffle = */ true};
 
         for (CNode* pnode : snap.Nodes()) {
             if (pnode->fDisconnect)

src/net_processing.cpp

@@ -4763,7 +4763,7 @@ void PeerManagerImpl::ProcessMessage(
             // the peer if the header turns out to be for an invalid block.
             // Note that if a peer tries to build on an invalid chain, that
             // will be detected and the peer will be disconnected/discouraged.
-            return ProcessHeadersMessage(pfrom, *peer, {cmpctblock.header}, /*punish_duplicate_invalid=*/true);
+            return ProcessHeadersMessage(pfrom, *peer, {cmpctblock.header}, /*via_compact_block=*/true);
         }
 
         if (fBlockReconstructed) {

src/netaddress.cpp

@@ -19,9 +19,6 @@
 #include <ios>
 #include <tuple>
 
-constexpr size_t CNetAddr::V1_SERIALIZATION_SIZE;
-constexpr size_t CNetAddr::MAX_ADDRV2_SIZE;
-
 CNetAddr::BIP155Network CNetAddr::GetBIP155Network() const
 {
     switch (m_net) {

src/netbase.cpp

@@ -658,7 +658,7 @@ bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_
             return false;
         }
     } else {
-        if (!Socks5(strDest, port, 0, sock)) {
+        if (!Socks5(strDest, port, nullptr, sock)) {
             return false;
         }
     }