Merge bitcoin/bitcoin#31308: ci, iwyu: Treat warnings as errors for specific directories

02d2b5a11c ci, iwyu: Treat warnings as errors for specific directories (Hennadii Stepanov)
57a3eac387 refactor: Fix includes in `index` directory (Hennadii Stepanov)
bdb8eadcdc refactor: Fix includes in `crypto` directory (Hennadii Stepanov)
56f2a689a2 ci: Do not patch `leveldb` to workaround UB in "tidy" CI job (Hennadii Stepanov)

Pull request description:

  This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.

ACKs for top commit:
  maflcko:
    re-ACK 02d2b5a11c 💮
  ryanofsky:
    Code review ACK 02d2b5a11c. Just rebased and update tidy patch comment again since last review
  willcl-ark:
    ACK 02d2b5a11c

Tree-SHA512: 1c966e01c47bf3e7d225faa3b819367f757430e2d71e9582fa82d67307aabe3f0d76f69346ee180192e7f5ab194ecc58d2b8ecf178eab26ba3309a6b55bff4b6

ci/test/03_test_script.sh

@@ -41,7 +41,10 @@ echo "=== BEGIN env ==="
 env
 echo "=== END env ==="
 
-(
+# Don't apply patches in the tidy job, because it relies on the `git diff`
+# command to detect IWYU errors. It is safe to skip this patch in the tidy job
+# because it doesn't run a UB detector.
+if [ "$RUN_TIDY" != "true" ]; then
   # compact->outputs[i].file_size is uninitialized memory, so reading it is UB.
   # The statistic bytes_written is only used for logging, which is disabled in
   # CI, so as a temporary minimal fix to work around UB and CI failures, leave
@@ -62,7 +65,7 @@ echo "=== END env ==="
    mutex_.Lock();
    stats_[compact->compaction->level() + 1].Add(stats);
 EOF
-)
+fi
 
 if [ "$RUN_FUZZ_TESTS" = "true" ]; then
   export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_corpora/
@@ -211,14 +214,30 @@ if [ "${RUN_TIDY}" = "true" ]; then
     false
   fi
 
+  # TODO: Consider enforcing IWYU across the entire codebase.
+  FILES_WITH_ENFORCED_IWYU="/src/(crypto|index)/.*\\.cpp"
+  jq --arg patterns "$FILES_WITH_ENFORCED_IWYU" 'map(select(.file | test($patterns)))' "${BASE_BUILD_DIR}/compile_commands.json" > "${BASE_BUILD_DIR}/compile_commands_iwyu_errors.json"
+  jq --arg patterns "$FILES_WITH_ENFORCED_IWYU" 'map(select(.file | test($patterns) | not))' "${BASE_BUILD_DIR}/compile_commands.json" > "${BASE_BUILD_DIR}/compile_commands_iwyu_warnings.json"
+
   cd "${BASE_ROOT_DIR}"
-  python3 "/include-what-you-use/iwyu_tool.py" \
+
-           -p "${BASE_BUILD_DIR}" "${MAKEJOBS}" \
+  run_iwyu() {
-           -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
+    mv "${BASE_BUILD_DIR}/$1" "${BASE_BUILD_DIR}/compile_commands.json"
-           -Xiwyu --max_line_length=160 \
+    python3 "/include-what-you-use/iwyu_tool.py" \
-           2>&1 | tee /tmp/iwyu_ci.out
+             -p "${BASE_BUILD_DIR}" "${MAKEJOBS}" \
-  cd "${BASE_ROOT_DIR}/src"
+             -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
-  python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
+             -Xiwyu --max_line_length=160 \
+             2>&1 | tee /tmp/iwyu_ci.out
+    python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
+  }
+
+  run_iwyu "compile_commands_iwyu_errors.json"
+  if ! ( git --no-pager diff --exit-code ); then
+    echo "^^^ ⚠️ Failure generated from IWYU"
+    false
+  fi
+
+  run_iwyu "compile_commands_iwyu_warnings.json"
   git --no-pager diff
 fi
 

contrib/devtools/iwyu/bitcoin.core.imp

@@ -1,3 +1,16 @@
-# Nothing for now.
 [
+  # Compiler intrinsics.
+  # See: https://github.com/include-what-you-use/include-what-you-use/issues/1764.
+  { "include": [ "<emmintrin.h>", "private", "<immintrin.h>", "public" ] },
+  { "include": [ "<smmintrin.h>", "private", "<immintrin.h>", "public" ] },
+  { "include": [ "<tmmintrin.h>", "private", "<immintrin.h>", "public" ] },
+
+  # libc symbols.
+  { "symbol": ["AT_HWCAP", "private", "<sys/auxv.h>", "public"] },
+  { "symbol": ["AT_HWCAP2", "private", "<sys/auxv.h>", "public"] },
+
+  # Fixed in https://github.com/include-what-you-use/include-what-you-use/pull/1706.
+  { "symbol": ["SEEK_CUR", "private", "<cstdio>", "public"] },
+  { "symbol": ["SEEK_END", "private", "<cstdio>", "public"] },
+  { "symbol": ["SEEK_SET", "private", "<cstdio>", "public"] },
 ]

src/crypto/chacha20.cpp

@@ -8,11 +8,10 @@
 #include <crypto/common.h>
 #include <crypto/chacha20.h>
 #include <support/cleanse.h>
-#include <span.h>
 
 #include <algorithm>
 #include <bit>
-#include <cstring>
+#include <cassert>
 
 #define QUARTERROUND(a,b,c,d) \
   a += b; d = std::rotl(d ^ a, 16); \

src/crypto/chacha20.h

@@ -5,12 +5,11 @@
 #ifndef BITCOIN_CRYPTO_CHACHA20_H
 #define BITCOIN_CRYPTO_CHACHA20_H
 
-#include <span.h>
-
 #include <array>
 #include <cstddef>
 #include <cstdint>
-#include <cstdlib>
+#include <iterator>
+#include <span>
 #include <utility>
 
 // classes for ChaCha20 256-bit stream cipher developed by Daniel J. Bernstein

src/crypto/chacha20poly1305.h

@@ -7,10 +7,10 @@
 
 #include <cstddef>
 #include <cstdint>
+#include <span>
 
 #include <crypto/chacha20.h>
 #include <crypto/poly1305.h>
-#include <span.h>
 
 /** The AEAD_CHACHA20_POLY1305 authenticated encryption algorithm from RFC8439 section 2.8. */
 class AEADChaCha20Poly1305

src/crypto/hex_base.cpp

@@ -5,8 +5,10 @@
 #include <crypto/hex_base.h>
 
 #include <array>
+#include <cassert>
 #include <cstring>
 #include <string>
+#include <tuple>
 
 namespace {
 

src/crypto/hex_base.h

@@ -9,6 +9,7 @@
 
 #include <cstddef>
 #include <cstdint>
+#include <span>
 #include <string>
 
 /**

src/crypto/hkdf_sha256_32.cpp

@@ -4,8 +4,9 @@
 
 #include <crypto/hkdf_sha256_32.h>
 
+#include <crypto/hmac_sha256.h>
+
 #include <cassert>
-#include <cstring>
 
 CHKDF_HMAC_SHA256_L32::CHKDF_HMAC_SHA256_L32(const unsigned char* ikm, size_t ikmlen, const std::string& salt)
 {

src/crypto/hkdf_sha256_32.h

@@ -5,10 +5,8 @@
 #ifndef BITCOIN_CRYPTO_HKDF_SHA256_32_H
 #define BITCOIN_CRYPTO_HKDF_SHA256_32_H
 
-#include <crypto/hmac_sha256.h>
+#include <cstddef>
-
+#include <string>
-#include <cstdint>
-#include <cstdlib>
 
 /** A rfc5869 HKDF implementation with HMAC_SHA256 and fixed key output length of 32 bytes (L=32) */
 class CHKDF_HMAC_SHA256_L32

src/crypto/hmac_sha256.cpp

@@ -4,6 +4,8 @@
 
 #include <crypto/hmac_sha256.h>
 
+#include <crypto/sha256.h>
+
 #include <cstring>
 
 CHMAC_SHA256::CHMAC_SHA256(const unsigned char* key, size_t keylen)

src/crypto/hmac_sha256.h

@@ -7,8 +7,7 @@
 
 #include <crypto/sha256.h>
 
-#include <cstdint>
+#include <cstddef>
-#include <cstdlib>
 
 /** A hasher class for HMAC-SHA-256. */
 class CHMAC_SHA256

src/crypto/hmac_sha512.cpp

@@ -4,6 +4,8 @@
 
 #include <crypto/hmac_sha512.h>
 
+#include <crypto/sha512.h>
+
 #include <cstring>
 
 CHMAC_SHA512::CHMAC_SHA512(const unsigned char* key, size_t keylen)

src/crypto/hmac_sha512.h

@@ -7,8 +7,7 @@
 
 #include <crypto/sha512.h>
 
-#include <cstdint>
+#include <cstddef>
-#include <cstdlib>
 
 /** A hasher class for HMAC-SHA-512. */
 class CHMAC_SHA512

src/crypto/muhash.cpp

@@ -7,11 +7,12 @@
 #include <crypto/chacha20.h>
 #include <crypto/common.h>
 #include <hash.h>
+#include <span.h>
+#include <uint256.h>
 #include <util/check.h>
 
 #include <bit>
-#include <cassert>
+#include <cstring>
-#include <cstdio>
 #include <limits>
 
 namespace {

src/crypto/muhash.h

@@ -6,9 +6,12 @@
 #define BITCOIN_CRYPTO_MUHASH_H
 
 #include <serialize.h>
-#include <uint256.h>
 
+#include <cstddef>
 #include <cstdint>
+#include <span>
+
+class uint256;
 
 class Num3072
 {

src/crypto/poly1305.cpp

@@ -5,8 +5,6 @@
 #include <crypto/common.h>
 #include <crypto/poly1305.h>
 
-#include <cstring>
-
 namespace poly1305_donna {
 
 // Based on the public domain implementation by Andrew Moon

src/crypto/poly1305.h

@@ -8,8 +8,9 @@
 #include <span.h>
 
 #include <cassert>
+#include <cstddef>
 #include <cstdint>
-#include <cstdlib>
+#include <span>
 
 #define POLY1305_BLOCK_SIZE 16
 

src/crypto/sha256.cpp

@@ -10,7 +10,7 @@
 #include <cstring>
 
 #if !defined(DISABLE_OPTIMIZED_SHA256)
-#include <compat/cpuid.h>
+#include <compat/cpuid.h> // IWYU pragma: keep
 
 #if defined(__linux__) && defined(ENABLE_ARM_SHANI)
 #include <sys/auxv.h>

src/crypto/sha256_arm_shani.cpp

@@ -13,7 +13,6 @@
 #include <array>
 #include <cstdint>
 #include <cstddef>
-#include <arm_acle.h>
 #include <arm_neon.h>
 
 namespace {

src/crypto/sha256_sse4.cpp

@@ -5,11 +5,11 @@
 // This is a translation to GCC extended asm syntax from YASM code by Intel
 // (available at the bottom of this file).
 
+#if defined(__x86_64__) || defined(__amd64__)
+
 #include <cstdint>
 #include <cstdlib>
 
-#if defined(__x86_64__) || defined(__amd64__)
-
 namespace sha256_sse4
 {
 void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)

src/crypto/sha256_x86_shani.cpp

@@ -8,6 +8,7 @@
 
 #if defined(ENABLE_SSE41) && defined(ENABLE_X86_SHANI)
 
+#include <cstddef>
 #include <cstdint>
 #include <immintrin.h>
 

src/crypto/sha3.cpp

@@ -9,9 +9,9 @@
 #include <crypto/common.h>
 
 #include <algorithm>
-#include <array>
 #include <bit>
-#include <cstdint>
+#include <cassert>
+#include <iterator>
 #include <span>
 
 void KeccakF(uint64_t (&st)[25])

src/crypto/sha3.h

@@ -5,10 +5,9 @@
 #ifndef BITCOIN_CRYPTO_SHA3_H
 #define BITCOIN_CRYPTO_SHA3_H
 
-#include <span.h>
-
 #include <cstdint>
 #include <cstdlib>
+#include <span>
 
 //! The Keccak-f[1600] transform.
 void KeccakF(uint64_t (&st)[25]);

src/crypto/siphash.cpp

@@ -4,7 +4,11 @@
 
 #include <crypto/siphash.h>
 
+#include <uint256.h>
+
 #include <bit>
+#include <cassert>
+#include <span>
 
 #define SIPROUND do { \
     v0 += v1; v1 = std::rotl(v1, 13); v1 ^= v0; \

src/crypto/siphash.h

@@ -6,9 +6,9 @@
 #define BITCOIN_CRYPTO_SIPHASH_H
 
 #include <cstdint>
+#include <span>
 
-#include <span.h>
+class uint256;
-#include <uint256.h>
 
 /** SipHash-2-4 */
 class CSipHasher

src/index/base.cpp

@@ -2,10 +2,13 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <chainparams.h>
-#include <common/args.h>
 #include <index/base.h>
+
+#include <chain.h>
+#include <common/args.h>
+#include <dbwrapper.h>
 #include <interfaces/chain.h>
+#include <interfaces/types.h>
 #include <kernel/chain.h>
 #include <logging.h>
 #include <node/abort.h>
@@ -13,20 +16,31 @@
 #include <node/context.h>
 #include <node/database_args.h>
 #include <node/interface_ui.h>
+#include <primitives/block.h>
+#include <sync.h>
 #include <tinyformat.h>
+#include <uint256.h>
 #include <undo.h>
+#include <util/fs.h>
 #include <util/string.h>
 #include <util/thread.h>
+#include <util/threadinterrupt.h>
+#include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
+#include <validationinterface.h>
 
-#include <chrono>
+#include <cassert>
+#include <compare>
+#include <cstdint>
 #include <memory>
 #include <optional>
+#include <span>
 #include <stdexcept>
 #include <string>
 #include <thread>
 #include <utility>
+#include <vector>
 
 constexpr uint8_t DB_BEST_BLOCK{'B'};
 

src/index/base.h

@@ -5,29 +5,41 @@
 #ifndef BITCOIN_INDEX_BASE_H
 #define BITCOIN_INDEX_BASE_H
 
+#include <attributes.h>
 #include <dbwrapper.h>
 #include <interfaces/chain.h>
-#include <interfaces/types.h>
+#include <kernel/cs_main.h>
-#include <util/string.h>
+#include <threadsafety.h>
+#include <uint256.h>
+#include <util/fs.h>
 #include <util/threadinterrupt.h>
 #include <validationinterface.h>
 
+#include <atomic>
+#include <cstddef>
+#include <memory>
+#include <optional>
 #include <string>
+#include <thread>
 
 class CBlock;
 class CBlockIndex;
 class Chainstate;
-class ChainstateManager;
-namespace interfaces {
-class Chain;
-} // namespace interfaces
 
+struct CBlockLocator;
 struct IndexSummary {
     std::string name;
     bool synced{false};
     int best_block_height{0};
     uint256 best_block_hash;
 };
+namespace interfaces {
+struct BlockRef;
+}
+namespace util {
+template <unsigned int num_params>
+struct ConstevalFormatString;
+}
 
 /**
  * Base class for indices of blockchain data. This implements

src/index/blockfilterindex.cpp

@@ -2,19 +2,39 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <map>
+#include <index/blockfilterindex.h>
 
-#include <clientversion.h>
+#include <blockfilter.h>
+#include <chain.h>
 #include <common/args.h>
 #include <dbwrapper.h>
+#include <flatfile.h>
 #include <hash.h>
-#include <index/blockfilterindex.h>
+#include <index/base.h>
+#include <interfaces/chain.h>
+#include <interfaces/types.h>
 #include <logging.h>
-#include <node/blockstorage.h>
+#include <serialize.h>
-#include <undo.h>
+#include <streams.h>
-#include <util/fs_helpers.h>
+#include <sync.h>
+#include <uint256.h>
+#include <util/check.h>
+#include <util/fs.h>
+#include <util/hasher.h>
 #include <util/syserror.h>
 
+#include <cerrno>
+#include <exception>
+#include <ios>
+#include <map>
+#include <optional>
+#include <span>
+#include <stdexcept>
+#include <string>
+#include <tuple>
+#include <utility>
+#include <vector>
+
 /* The index database stores three items for each block: the disk location of the encoded filter,
  * its dSHA256 hash, and the header. Those belonging to blocks on the active chain are indexed by
  * height, and those belonging to blocks that have been reorganized out of the active chain are

src/index/blockfilterindex.h

@@ -6,13 +6,24 @@
 #define BITCOIN_INDEX_BLOCKFILTERINDEX_H
 
 #include <attributes.h>
-#include <blockfilter.h>
-#include <chain.h>
 #include <flatfile.h>
 #include <index/base.h>
+#include <interfaces/chain.h>
+#include <sync.h>
+#include <uint256.h>
 #include <util/hasher.h>
 
+#include <cstddef>
+#include <cstdint>
+#include <functional>
+#include <memory>
+#include <optional>
 #include <unordered_map>
+#include <vector>
+
+class BlockFilter;
+class CBlockIndex;
+enum class BlockFilterType : uint8_t;
 
 static const char* const DEFAULT_BLOCKFILTERINDEX = "0";
 

src/index/coinstatsindex.cpp

@@ -2,20 +2,39 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <index/coinstatsindex.h>
+
 #include <arith_uint256.h>
+#include <chain.h>
 #include <chainparams.h>
 #include <coins.h>
 #include <common/args.h>
+#include <consensus/amount.h>
 #include <crypto/muhash.h>
-#include <index/coinstatsindex.h>
+#include <dbwrapper.h>
+#include <index/base.h>
+#include <interfaces/chain.h>
+#include <interfaces/types.h>
 #include <kernel/coinstats.h>
 #include <logging.h>
-#include <node/blockstorage.h>
+#include <primitives/block.h>
+#include <primitives/transaction.h>
+#include <script/script.h>
 #include <serialize.h>
-#include <txdb.h>
+#include <uint256.h>
 #include <undo.h>
+#include <util/check.h>
+#include <util/fs.h>
 #include <validation.h>
 
+#include <compare>
+#include <ios>
+#include <limits>
+#include <span>
+#include <string>
+#include <utility>
+#include <vector>
+
 using kernel::ApplyCoinHash;
 using kernel::CCoinsStats;
 using kernel::GetBogoSize;

src/index/coinstatsindex.h

@@ -6,11 +6,18 @@
 #define BITCOIN_INDEX_COINSTATSINDEX_H
 
 #include <arith_uint256.h>
+#include <consensus/amount.h>
 #include <crypto/muhash.h>
 #include <index/base.h>
+#include <interfaces/chain.h>
+#include <uint256.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <memory>
+#include <optional>
 
 class CBlockIndex;
-class CDBBatch;
 namespace kernel {
 struct CCoinsStats;
 }

src/index/txindex.cpp

@@ -4,14 +4,32 @@
 
 #include <index/txindex.h>
 
-#include <clientversion.h>
 #include <common/args.h>
+#include <dbwrapper.h>
+#include <flatfile.h>
+#include <index/base.h>
 #include <index/disktxpos.h>
+#include <interfaces/chain.h>
 #include <logging.h>
 #include <node/blockstorage.h>
-#include <primitives/transaction_identifier.h>
+#include <primitives/block.h>
+#include <primitives/transaction.h>
+#include <serialize.h>
+#include <streams.h>
+#include <uint256.h>
+#include <util/fs.h>
 #include <validation.h>
 
+#include <cassert>
+#include <cstdint>
+#include <cstdio>
+#include <exception>
+#include <iterator>
+#include <span>
+#include <string>
+#include <utility>
+#include <vector>
+
 constexpr uint8_t DB_TXINDEX{'t'};
 
 std::unique_ptr<TxIndex> g_txindex;

src/index/txindex.h

@@ -6,6 +6,15 @@
 #define BITCOIN_INDEX_TXINDEX_H
 
 #include <index/base.h>
+#include <primitives/transaction.h>
+
+#include <cstddef>
+#include <memory>
+
+class uint256;
+namespace interfaces {
+class Chain;
+}
 
 static constexpr bool DEFAULT_TXINDEX{false};