Merge bitcoin/bitcoin#29408: lint: Check for missing bitcoin-config.h includes

fa58ae7 refactor: Add missing include for USE_BDB, USE_SQLITE to bench/wallet_ismine.cpp (MarcoFalke)
fa31908 lint: Check for missing or redundant bitcoin-config.h includes (MarcoFalke)
fa63b0e lint: Make lint error easier to spot in output (MarcoFalke)
fa770fd doc: Add missing RUST_BACKTRACE=1 (MarcoFalke)
fa10051 lint: Add get_subtrees() helper (MarcoFalke)

Pull request description:

  Missing `bitcoin-config.h` includes are problematic, because the build could silently pass, but produce an unintended result. For example, a slower fallback algorithm could be picked, even though `bitcoin-config.h` indicates that a faster feature is available and should be used.

  As the build succeeds silently, this problem is not possible to detect with iwyu.

  Thus, fix this by using a linter based on grepping the source code.

ACKs for top commit:
  theuni:
    Weak ACK fa58ae7.
  TheCharlatan:
    ACK fa58ae7
  hebasto:
    ACK fa58ae7, tested on Ubuntu 23.10 -- it catches bugs properly. I didn't review rust code changes.

Tree-SHA512: cf4346f81ea5b8c215da6004cb2403d1aaf569589613c305d8ba00329b82b3841da94fe1a69815ce15f2edecbef9b031758ec9b6433564976190e3cf91ec8181

Author: fanquake

ci/lint/04_install.sh

@@ -10,10 +10,11 @@ export PATH=$PWD/ci/retry:$PATH
 
 ${CI_RETRY_EXE} apt-get update
 # Lint dependencies:
+# - automake pkg-config libtool (for lint_includes_build_config)
 # - curl/xz-utils (to install shellcheck)
 # - git (used in many lint scripts)
 # - gpg (used by verify-commits)
-${CI_RETRY_EXE} apt-get install -y curl xz-utils git gpg
+${CI_RETRY_EXE} apt-get install -y automake pkg-config libtool curl xz-utils git gpg
 
 PYTHON_PATH="/python_build"
 if [ ! -d "${PYTHON_PATH}/bin" ]; then

src/bench/wallet_ismine.cpp

@@ -2,6 +2,9 @@
 // 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 // HAVE_CONFIG_H
 #include <bench/bench.h>
 #include <interfaces/chain.h>
 #include <key.h>
@@ -11,9 +14,9 @@
 #include <util/translation.h>
 #include <validationinterface.h>
 #include <wallet/context.h>
+#include <wallet/test/util.h>
 #include <wallet/wallet.h>
 #include <wallet/walletutil.h>
-#include <wallet/test/util.h>
 
 namespace wallet {
 static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_combo = 0)

test/lint/README.md

@@ -19,7 +19,7 @@ test runner
 To run all the lint checks in the test runner outside the docker, use:
 
 ```sh
-( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run )
+( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )
 ```
 
 #### Dependencies

test/lint/test_runner/src/main.rs

@@ -4,7 +4,7 @@
 
 use std::env;
 use std::fs;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::process::ExitCode;
 
@@ -34,17 +34,30 @@ fn get_git_root() -> PathBuf {
     PathBuf::from(check_output(git().args(["rev-parse", "--show-toplevel"])).unwrap())
 }
 
+/// Return all subtree paths
+fn get_subtrees() -> Vec<&'static str> {
+    vec![
+        "src/crc32c",
+        "src/crypto/ctaes",
+        "src/leveldb",
+        "src/minisketch",
+        "src/secp256k1",
+    ]
+}
+
+/// Return the pathspecs to exclude all subtrees
+fn get_pathspecs_exclude_subtrees() -> Vec<String> {
+    get_subtrees()
+        .iter()
+        .map(|s| format!(":(exclude){}", s))
+        .collect()
+}
+
 fn lint_subtree() -> LintResult {
     // This only checks that the trees are pure subtrees, it is not doing a full
     // check with -r to not have to fetch all the remotes.
     let mut good = true;
-    for subtree in [
-        "src/crypto/ctaes",
-        "src/secp256k1",
-        "src/minisketch",
-        "src/leveldb",
-        "src/crc32c",
-    ] {
+    for subtree in get_subtrees() {
         good &= Command::new("test/lint/git-subtree-check.sh")
             .arg(subtree)
             .status()
@@ -82,6 +95,102 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted.
     }
 }
 
+fn lint_includes_build_config() -> LintResult {
+    let config_path = "./src/config/bitcoin-config.h.in";
+    let include_directive = "#include <config/bitcoin-config.h>";
+    if !Path::new(config_path).is_file() {
+        assert!(Command::new("./autogen.sh")
+            .status()
+            .expect("command error")
+            .success());
+    }
+    let defines_regex = format!(
+        r"^\s*(?!//).*({})",
+        check_output(Command::new("grep").args(["undef ", "--", config_path]))
+            .expect("grep failed")
+            .lines()
+            .map(|line| {
+                line.split("undef ")
+                    .nth(1)
+                    .unwrap_or_else(|| panic!("Could not extract name in line: {line}"))
+            })
+            .collect::<Vec<_>>()
+            .join("|")
+    );
+    let print_affected_files = |mode: bool| {
+        // * mode==true: Print files which use the define, but lack the include
+        // * mode==false: Print files which lack the define, but use the include
+        let defines_files = check_output(
+            git()
+                .args([
+                    "grep",
+                    "--perl-regexp",
+                    if mode {
+                        "--files-with-matches"
+                    } else {
+                        "--files-without-match"
+                    },
+                    &defines_regex,
+                    "--",
+                    "*.cpp",
+                    "*.h",
+                ])
+                .args(get_pathspecs_exclude_subtrees())
+                .args([
+                    // These are exceptions which don't use bitcoin-config.h, rather the Makefile.am adds
+                    // these cppflags manually.
+                    ":(exclude)src/crypto/sha256_arm_shani.cpp",
+                    ":(exclude)src/crypto/sha256_avx2.cpp",
+                    ":(exclude)src/crypto/sha256_sse41.cpp",
+                    ":(exclude)src/crypto/sha256_x86_shani.cpp",
+                ]),
+        )
+        .expect("grep failed");
+        git()
+            .args([
+                "grep",
+                if mode {
+                    "--files-without-match"
+                } else {
+                    "--files-with-matches"
+                },
+                include_directive,
+                "--",
+            ])
+            .args(defines_files.lines())
+            .status()
+            .expect("command error")
+            .success()
+    };
+    let missing = print_affected_files(true);
+    if missing {
+        return Err(format!(
+            r#"
+^^^
+One or more files use a symbol declared in the bitcoin-config.h header. However, they are not
+including the header. This is problematic, because the header may or may not be indirectly
+included. If the indirect include were to be intentionally or accidentally removed, the build could
+still succeed, but silently be buggy. For example, a slower fallback algorithm could be picked,
+even though bitcoin-config.h indicates that a faster feature is available and should be used.
+
+If you are unsure which symbol is used, you can find it with this command:
+git grep --perl-regexp '{}' -- file_name
+            "#,
+            defines_regex
+        ));
+    }
+    let redundant = print_affected_files(false);
+    if redundant {
+        return Err(r#"
+^^^
+None of the files use a symbol declared in the bitcoin-config.h header. However, they are including
+the header. Consider removing the unused include.
+            "#
+        .to_string());
+    }
+    Ok(())
+}
+
 fn lint_doc() -> LintResult {
     if Command::new("test/lint/check-doc.py")
         .status()
@@ -123,6 +232,7 @@ fn main() -> ExitCode {
     let test_list: Vec<(&str, LintFn)> = vec![
         ("subtree check", lint_subtree),
         ("std::filesystem check", lint_std_filesystem),
+        ("build config includes check", lint_includes_build_config),
         ("-help=1 documentation check", lint_doc),
         ("lint-*.py scripts", lint_all),
     ];
@@ -134,7 +244,7 @@ fn main() -> ExitCode {
         // chdir to root before each lint test
         env::set_current_dir(&git_root).unwrap();
         if let Err(err) = lint_fn() {
-            println!("{err}\n^---- Failure generated from {lint_name}!");
+            println!("{err}\n^---- ⚠️ Failure generated from {lint_name}!");
             test_failed = true;
         }
     }