Merge bitcoin/bitcoin#33142: test: Run bench sanity checks in parallel with functional tests

fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e test: Run bench sanity checks in parallel with functional tests (MarcoFalke)
fa9fdbce7928e1ce8c5e17d965ff9bcc1282c8f5 test: Pass bench exe into test framework utils (MarcoFalke)

Pull request description:

  The ctest target `bench_sanity_check` has many issues:

  * With sanitizers enabled, it is one of the slowest targets, often taking several minutes. See https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-2984264066.
  * There is no insight from ctest into how long each individual sanity check takes.
  * On a timeout, or OOM issue, there is no insight into which sub-bench failed. The failure will generally just look like `75/153 Test   #9: bench_sanity_check ...................***Failed  770.84 sec    out of memory`
  * Places that can't use ctest (like the Windows-cross CI task) have to explicitly run it, or risk forgetting to run it.
  * All benchmarks are run sequentially, when they could run in parallel instead.

  Both issues can lead to CI timeouts and leave CPU unused during testing.

  Fix all issues by running it as part of the functional tests instead. This is similar to the rpcauth tests (https://github.com/bitcoin/bitcoin/pull/32881) and util tests [bitcoin-tx, and bitcoin-util] (https://github.com/bitcoin/bitcoin/pull/32697).

ACKs for top commit:
  achow101:
    ACK fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e
  l0rinc:
    Tested ACK fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e
  janb84:
    tACK fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e
  willcl-ark:
    ACK fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e

Tree-SHA512: d27e363b7896a7543a4ee8df41a56e58b74f07d4f296e2e5ee293fc91817d0be310e26905755fb94d44417d94fa29ad4cc5d4aa19e78d25d41bc2d9e0948c034

.github/workflows/ci.yml

@@ -320,6 +320,7 @@ jobs:
           BITCOIN_BIN: '${{ github.workspace }}\build\bin\Release\bitcoin.exe'
           BITCOIND: '${{ github.workspace }}\build\bin\Release\bitcoind.exe'
           BITCOINCLI: '${{ github.workspace }}\build\bin\Release\bitcoin-cli.exe'
+          BITCOIN_BENCH: '${{ github.workspace }}\build\bin\Release\bench_bitcoin.exe'
           BITCOINTX: '${{ github.workspace }}\build\bin\Release\bitcoin-tx.exe'
           BITCOINUTIL: '${{ github.workspace }}\build\bin\Release\bitcoin-util.exe'
           BITCOINWALLET: '${{ github.workspace }}\build\bin\Release\bitcoin-wallet.exe'
@@ -483,9 +484,6 @@ jobs:
           ./src/univalue/object.exe
           ./src/univalue/unitester.exe
 
-      - name: Run benchmarks
-        run: ./bin/bench_bitcoin.exe -sanity-check
-
       - name: Adjust paths in test/config.ini
         shell: pwsh
         run: |

src/bench/CMakeLists.txt

@@ -84,8 +84,4 @@ if(ENABLE_WALLET)
   target_link_libraries(bench_bitcoin bitcoin_wallet)
 endif()
 
-add_test(NAME bench_sanity_check
-  COMMAND bench_bitcoin -sanity-check
-)
-
 install_binary_component(bench_bitcoin INTERNAL)

test/CMakeLists.txt

@@ -16,6 +16,7 @@ function(create_test_config)
   endmacro()
 
   set_configure_variable(ENABLE_WALLET ENABLE_WALLET)
+  set_configure_variable(BUILD_BENCH BUILD_BENCH)
   set_configure_variable(BUILD_CLI BUILD_BITCOIN_CLI)
   set_configure_variable(BUILD_TX BUILD_BITCOIN_TX)
   set_configure_variable(BUILD_UTIL BUILD_BITCOIN_UTIL)

test/config.ini.in

@@ -16,6 +16,7 @@ RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py
 [components]
 # Which components are enabled. These are commented out by cmake if they were disabled during configuration.
 @ENABLE_WALLET_TRUE@ENABLE_WALLET=true
+@BUILD_BENCH_TRUE@BUILD_BENCH=true
 @BUILD_BITCOIN_CLI_TRUE@ENABLE_CLI=true
 @BUILD_BITCOIN_TX_TRUE@BUILD_BITCOIN_TX=true
 @BUILD_BITCOIN_UTIL_TRUE@ENABLE_BITCOIN_UTIL=true

test/functional/test_framework/test_framework.py

@@ -943,6 +943,11 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         if not self.is_bitcoin_chainstate_compiled():
             raise SkipTest("bitcoin-chainstate has not been compiled")
 
+    def skip_if_no_bitcoin_bench(self):
+        """Skip the running test if bench_bitcoin has not been compiled."""
+        if not self.is_bench_compiled():
+            raise SkipTest("bench_bitcoin has not been compiled")
+
     def skip_if_no_cli(self):
         """Skip the running test if bitcoin-cli has not been compiled."""
         if not self.is_cli_compiled():
@@ -976,6 +981,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         if self.options.valgrind:
             raise SkipTest("This test is not compatible with Valgrind.")
 
+    def is_bench_compiled(self):
+        """Checks whether bench_bitcoin was compiled."""
+        return self.config["components"].getboolean("BUILD_BENCH")
+
     def is_cli_compiled(self):
         """Checks whether bitcoin-cli was compiled."""
         return self.config["components"].getboolean("ENABLE_CLI")

test/functional/test_framework/util.py

@@ -261,6 +261,10 @@ class Binaries:
         # Add -nonamed because "bitcoin rpc" enables -named by default, but bitcoin-cli doesn't
         return self._argv("rpc", self.paths.bitcoincli) + ["-nonamed"]
 
+    def bench_argv(self):
+        "Return argv array that should be used to invoke bench_bitcoin"
+        return self._argv("bench", self.paths.bitcoin_bench)
+
     def tx_argv(self):
         "Return argv array that should be used to invoke bitcoin-tx"
         return self._argv("tx", self.paths.bitcointx)
@@ -301,6 +305,7 @@ def get_binary_paths(config):
     binaries = {
         "bitcoin": "BITCOIN_BIN",
         "bitcoind": "BITCOIND",
+        "bench_bitcoin": "BITCOIN_BENCH",
         "bitcoin-cli": "BITCOINCLI",
         "bitcoin-util": "BITCOINUTIL",
         "bitcoin-tx": "BITCOINTX",

test/functional/test_runner.py

@@ -29,6 +29,11 @@ import sys
 import tempfile
 import re
 import logging
+from test_framework.util import (
+    Binaries,
+    export_env_build_path,
+    get_binary_paths,
+)
 
 # Minimum amount of space to run the tests.
 MIN_FREE_SPACE = 1.1 * 1024 * 1024 * 1024
@@ -87,7 +92,12 @@ EXTENDED_SCRIPTS = [
     'feature_index_prune.py',
 ]
 
+# Special script to run each bench sanity check
+TOOL_BENCH_SANITY_CHECK = "tool_bench_sanity_check.py"
+
 BASE_SCRIPTS = [
+    # Special scripts that are "expanded" later
+    TOOL_BENCH_SANITY_CHECK,
     # Scripts that are run by default.
     # Longest test should go first, to favor running tests in parallel
     # vv Tests less than 5m vv
@@ -458,6 +468,8 @@ def main():
         print("Re-compile with the -DBUILD_DAEMON=ON build option")
         sys.exit(1)
 
+    export_env_build_path(config)
+
     # Build tests
     test_list = deque()
     if tests:
@@ -512,6 +524,15 @@ def main():
                 # Exclude all variants of a test
                 remove_tests([test for test in test_list if test.split('.py')[0] == exclude_test.split('.py')[0]])
 
+    if config["components"].getboolean("BUILD_BENCH") and TOOL_BENCH_SANITY_CHECK in test_list:
+        # Remove it, and expand it for each bench in the list
+        test_list.remove(TOOL_BENCH_SANITY_CHECK)
+        bench_cmd = Binaries(get_binary_paths(config), bin_dir=None).bench_argv() + ["-list"]
+        bench_list = subprocess.check_output(bench_cmd, text=True).splitlines()
+        bench_list = [f"{TOOL_BENCH_SANITY_CHECK} --bench={b}" for b in bench_list]
+        # Start with special scripts (variable, unknown runtime)
+        test_list.extendleft(reversed(bench_list))
+
     if args.filter:
         test_list = deque(filter(re.compile(args.filter).search, test_list))
 

test/functional/tool_bench_sanity_check.py

@@ -0,0 +1,41 @@
+#!/usr/bin/env python3
+# Copyright (c) The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or https://opensource.org/license/mit/.
+"""Special script to run each bench sanity check
+"""
+import shlex
+import subprocess
+
+from test_framework.test_framework import BitcoinTestFramework
+
+
+class BenchSanityCheck(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 0  # No node/datadir needed
+
+    def setup_network(self):
+        pass
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_bitcoin_bench()
+
+    def add_options(self, parser):
+        parser.add_argument(
+            "--bench",
+            default=".*",
+            help="Regex to filter the bench to run (default=%(default)s)",
+        )
+
+    def run_test(self):
+        cmd = self.get_binaries().bench_argv() + [
+            f"-filter={self.options.bench}",
+            "-sanity-check",
+        ]
+        self.log.info(f"Starting: {shlex.join(cmd)}")
+        subprocess.run(cmd, check=True)
+        self.log.info("Success!")
+
+
+if __name__ == "__main__":
+    BenchSanityCheck(__file__).main()