Remove boost as a unit test runner #35587

tl;dr: Make an equivalent test runner to boost with simpler macros and tighter coupling to bitcoin-specific code.

Motivation

There are a number of problems with using dependencies in general. Bugs must either be promptly upstreamed or patched, projects may be abandoned, and they are not tailored to a particular use case. This PR removes boost for writing and running unit tests. In the case of Boost.Test, there are a number of issues which are listed below.

My problems with Boost.Test

Macros

When using boost, there are 3 ways of writing the same thing, each with different outcomes

• BOOST_CHECK(expr1 == expr2): failures show the expression, but no values for expr1 and expr2
• BOOST_CHECK_EQUAL(expr1, expr2): failures show the expression and values, but requires developers remember CHECK_EQUAL and does not use usual operands
• BOOST_TEST(expr1 == expr2): failures show the expression and values, and developers may use familiar operands (==, !=)

All three are preserved to maintain backwards compatibility - presumably, but this results in all 3 being used throughout the repository. As a bonus there is a 4th way, with added context

• BOOST_CHECK_MESSAGE(expr1 == expr2, "the context"): failures show the expression and message, but no values for expr1 and expr2

To make debugging and review easier, all of these should be unified to a single macro. Test sites use operands, print helpful messages - values are always included, and do not vary in approach (deciding between BOOST_TEST, CHECK, and BOOST_CHECK_EQUAL is unintuitive)

<details> <summary>Example boost outputs</summary>

BOOST_AUTO_TEST_CASE(mustfail)
{
    auto a{1};
    auto b{2};
    BOOST_CHECK(a == b);
    BOOST_CHECK_EQUAL(a, b);
    BOOST_TEST(a == b);
    COutPoint outpoint_1{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 0};
    COutPoint outpoint_2{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 1};
    BOOST_CHECK(outpoint_1 == outpoint_2);
    // Doesn't even compile
    // BOOST_TEST(outpoint_1 == outpoint_2);
    // BOOST_CHECK_EQUAL(outpoint_1, outpoint_2);
}

Running 1 test case...
test/argsman_tests.cpp(31): error: in "argsman_tests/mustfail": check a == b has failed
test/argsman_tests.cpp(32): error: in "argsman_tests/mustfail": check a == b has failed [1 != 2]
test/argsman_tests.cpp(33): error: in "argsman_tests/mustfail": check a == b has failed [1 != 2]
test/argsman_tests.cpp(36): error: in "argsman_tests/mustfail": check outpoint_1 == outpoint_2 has failed

</details>

<details> <summary>New outputs</summary>

TEST_CASE(mustfail)
{
    auto a{1};
    auto b{2};
    CHECK(a == b);
    COutPoint outpoint_1{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 0};
    COutPoint outpoint_2{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 1};
    CHECK(outpoint_1 == outpoint_2);
}

Running 1 test cases...
[FAIL]: test/argsman_tests.cpp:30: CHECK(a == b)
1 == 2

[FAIL]: test/argsman_tests.cpp:33: CHECK(outpoint_1 == outpoint_2)
COutPoint(0000000000, 0) == COutPoint(0000000000, 1)

[FAIL] mustfail (2/2 checks failed)

</details>

No repository context

Many types implement ToString, but boost has no way of meaningfully using these representations (will change w/ future std::format). With a repository-specific test runner we can use these today. See example above.

Not extensible

With the test runner in this repository we can add things such as #35139, implement string representations for more types, and implement ideas from #8670.

Other issues

More issues with boost are raised in #34666, #8670. This PR does not resolve all of them, but one it does address immediately is banning comparisons of integers with different signs, previously allowed by boost. Using a test runner within the source also makes IWYU easier to work with.

High level changes

For those running tests, not a tremendous amount has changed in this PR. For instance, the doc page remains valid. Changes to the runner options include anything that is not help, run_test, log_level, list_content. This includes catch_system_errors which is always no. The log_levels have been reduced to 5 levels.

Changes in writing tests

• CHECK to compare two values with any ==, !=, >, etc
• CHECK(a == b, "my message") to append a message
• REQUIRE, same as CHECK, but will fail the test immediately
• CHECK_EQUAL_RANGES(it1, it2) to compare two ranges
• TEST_CASE(name) to add a test
• FIXTURE_TEST_CASE(name, Fixture) to add a test with a fixture
• TEST_SUITE_BEGIN/END to declare a suite

FIXTURE_TEST_SUITE_* is intentionally omitted. The readability of this macro is unclear as to if all unit tests share the same fixture or reinitialize the fixture. All test cases should be declared with FIXTURE if they use one.

Anticipated questions

Q: Why not name the macros BOOST_*? A: This would defeat the advantages of using a new framework, which unifies CHECK calls. We should just rip the bandaid off now.

Q: Why not run the last commit as a scripted-diff? A: The migration script is essentially a small tokenizer and would be more technically difficult to review. IMO it is best to review each callsite change, albeit many of them. The script is still posted below for reviewers to confirm the last commit is entirely mechanical, if desired.

Q: Why no concurrency? A: The unit tests rely on the "current test case," so this would have to change to adopt a concurrent runner. I considered this out of scope for an already large change.

Migration

HEAD~2 is the addition of the framework. Most commits are small but do not have much overlap. 1-16 are all small and disjoint commits that prepare for the migration.

<details> <summary>Migration script</summary>

import re
import sys
from pathlib import Path

_TOKEN_RE = re.compile(
    r'"(?:\\.|[^"\\])*"'
    r"|(?<![0-9a-fA-F'])'(?:\\.|[^'\\])*'"
    r"|(?P<open>[(\[{])"
    r"|(?P<close>[)\]}])"
    r"|(?P<id>[A-Za-z_][A-Za-z0-9_]*)"
    r"|(?P<op>&&|\|\||==|!=|<=|>=)"
    r"|(?P<other>.)",
    re.DOTALL,
)

def _walk(text, start=0):
    depth = 0
    for m in _TOKEN_RE.finditer(text, start):
        kind = m.lastgroup
        if kind == "close":
            depth -= 1
        yield depth, m
        if kind == "open":
            depth += 1

def find_close(text, open_idx):
    for depth, m in _walk(text, open_idx):
        if m.lastgroup == "close" and depth == 0:
            return m.end()

def split_args(args):
    parts, prev = [], 0
    for depth, m in _walk(args):
        if depth == 0 and m.lastgroup == "other" and m.group() == ",":
            parts.append(args[prev:m.start()])
            prev = m.end()
    parts.append(args[prev:])
    return parts

_ALWAYS = {"&&", "||", "?", "|", "&", "^"}
_COMPARE = {"==", "!=", "<=", ">="}

def has_top_level(text, ops):
    for depth, m in _walk(text):
        if depth != 0:
            continue
        tok = m.group()
        if tok not in ops:
            continue
        if m.lastgroup == "op" or tok == "?":
            return True
        if tok in "|&^" and text[m.end():m.end() + 1] != "=":
            prev = text[:m.start()].rstrip()
            if prev and (prev[-1].isalnum() or prev[-1] in "_)]}\"'"):
                return True
    return False

def wrap(expr, ops):
    expr = expr.strip()
    return f"({expr})" if has_top_level(expr, ops) else expr

_IDENT_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
_BOOST_RE = re.compile(r"\b(BOOST_[A-Z0-9_]+)\s*\(")
_FILE_NS_RE = re.compile(
    r"^[ \t]*namespace[ \t]+([A-Za-z_][A-Za-z0-9_]*)[ \t]*\{", re.MULTILINE
)

def ident(arg):
    name = arg.strip()
    if not _IDENT_RE.match(name):
        raise ValueError(f"not a valid C++ identifier: {name!r}")
    return name

def iter_pair(begin, end):
    """`X.begin()`/`X.end()` collapse to `X`; `->begin/end` to `*X`; else subrange."""
    b, e = begin.strip(), end.strip()
    for sep, deref in ((r"\.", ""), (r"->", "*")):
        mb = re.match(rf"^(.+){sep}begin\s*\(\s*\)$", b)
        me = re.match(rf"^(.+){sep}end\s*\(\s*\)$", e)
        if mb and me and (base := mb.group(1).strip()) == me.group(1).strip():
            return f"*({base})" if deref else base
    return f"std::ranges::subrange({b}, {e})"

_RENAMES = {
    "BOOST_CHECK_NO_THROW": "CHECK_NOTHROW",
    "BOOST_REQUIRE_NO_THROW": "REQUIRE_NOTHROW",
    "BOOST_CHECK_THROW": "CHECK_THROWS_AS",
    "BOOST_CHECK_EXCEPTION": "CHECK_EXCEPTION",
    "BOOST_TEST_MESSAGE": "TEST_MESSAGE",
    "BOOST_WARN_MESSAGE": "WARN_MESSAGE",
}

_COMPARE_OP = {
    f"BOOST_{k}_{n}": (k, o)
    for k in ("CHECK", "REQUIRE")
    for n, o in (("EQUAL", "=="), ("NE", "!="), ("LT", "<"), ("LE", "<="), ("GT", ">"), ("GE", ">="))
}

def rewrite_macros(text):
    while True:
        declared_ns = set(_FILE_NS_RE.findall(text))
        suite_stack = []
        out, cursor = [], 0

        for m in _BOOST_RE.finditer(text):
            start = m.start()
            if start < cursor:
                continue
            macro = m.group(1)
            close_end = find_close(text, m.end() - 1)
            if close_end is None:
                continue
            args = text[m.end():close_end - 1]
            replacement = None

            if macro in ("BOOST_CHECK", "BOOST_TEST", "BOOST_REQUIRE", "BOOST_TEST_REQUIRE"):
                target = "REQUIRE" if "REQUIRE" in macro else "CHECK"
                body = f"({args})" if has_top_level(args, _ALWAYS) else args
                replacement = f"{target}({body})"
            elif macro in _RENAMES:
                replacement = f"{_RENAMES[macro]}({args})"
            elif macro == "BOOST_ERROR":
                replacement = f"RECORD_ERROR({args.strip()})"
            elif macro in ("BOOST_CHECK_MESSAGE", "BOOST_REQUIRE_MESSAGE"):
                parts = split_args(args)
                if len(parts) >= 2:
                    target = "REQUIRE" if "REQUIRE" in macro else "CHECK"
                    replacement = f"{target}({wrap(parts[0], _ALWAYS)}, {','.join(parts[1:]).strip()})"
            elif macro in _COMPARE_OP:
                target, op = _COMPARE_OP[macro]
                parts = split_args(args)
                if len(parts) == 2:
                    a, b = wrap(parts[0], _ALWAYS | _COMPARE), wrap(parts[1], _ALWAYS | _COMPARE)
                    replacement = f"{target}({a} {op} {b})"
            elif macro == "BOOST_CHECK_EQUAL_COLLECTIONS":
                parts = split_args(args)
                if len(parts) == 4:
                    replacement = f"CHECK_EQUAL_RANGES({iter_pair(parts[0], parts[1])}, {iter_pair(parts[2], parts[3])})"
            elif macro == "BOOST_AUTO_TEST_CASE":
                name = ident(split_args(args)[0])
                fixture = suite_stack[-1][0] if suite_stack else None
                replacement = (f"FIXTURE_TEST_CASE({name}, {fixture})"
                               if fixture else f"TEST_CASE({name})")
            elif macro == "BOOST_FIXTURE_TEST_CASE":
                parts = split_args(args)
                if len(parts) >= 2:
                    replacement = f"FIXTURE_TEST_CASE({ident(parts[0])}, {parts[1].strip()})"
            elif macro in ("BOOST_AUTO_TEST_SUITE", "BOOST_FIXTURE_TEST_SUITE"):
                parts = split_args(args)
                name = ident(parts[0])
                fixture = parts[1].strip() if macro == "BOOST_FIXTURE_TEST_SUITE" else None
                # Re-open the namespace Boost's *_TEST_SUITE opened implicitly,
                # if the suite name matches a file-scope namespace.
                ns = name if name in declared_ns else None
                suite_stack.append((fixture, ns))
                prefix = f"namespace {ns} {{\n" if ns else ""
                replacement = f"{prefix}TEST_SUITE_BEGIN({name})"
            elif macro == "BOOST_AUTO_TEST_SUITE_END":
                ns = suite_stack.pop()[1] if suite_stack else None
                suffix = f"\n}} // namespace {ns}" if ns else ""
                replacement = f"TEST_SUITE_END(){suffix}"
            elif macro in ("BOOST_TEST_INFO", "BOOST_TEST_INFO_SCOPE"):
                replacement = f"/* {macro}({args}) */"

            if replacement is None:
                continue
            out.append(text[cursor:start])
            out.append(replacement)
            cursor = close_end

        out.append(text[cursor:])
        new = "".join(out)
        if new == text:
            return new
        text = new

def rewrite_includes(text):
    text = re.sub(r"^#include\s*<boost/test/[^>]+>\s*$",
                  "#include <test/util/framework.h>\n", text, flags=re.MULTILINE)
    text = re.sub(r"^#define\s+BOOST_TEST_MODULE\b.*$",
                  "#define BITCOIN_TEST_MAIN\n", text, flags=re.MULTILINE)
    text = re.sub(r"(?:#include <test/util/framework\.h>\n+)+",
                  "#include <test/util/framework.h>\n\n", text)
    return text.replace("boost::unit_test::framework::master_test_suite().argv[0]",
                        "framework::executable_path()")

_FILE_SWAPS = {
    "test/lint/lint-includes.py": [
        ("boost/test/included/unit_test.hpp", None),
        ("boost/test/unit_test.hpp", None),
    ],
    "test/lint/lint-tests.py": [
        ("grep_boost_test_suites", "grep_test_suites"),
        (r'r"^(BOOST_FIXTURE_TEST_SUITE|BOOST_AUTO_TEST_SUITE)\("', r'r"^TEST_SUITE_BEGIN\("'),
        (r'r"/(.*?)\.cpp:(?:BOOST_FIXTURE_TEST_SUITE|BOOST_AUTO_TEST_SUITE)\(\1(_[a-z0-9]+)?[,)]"',
         r'r"/(.*?)\.cpp:TEST_SUITE_BEGIN\(\1(_[a-z0-9]+)?\)"'),
        (r're.search(r"\((.*?)[,)]", x)', r're.search(r"\((.*?)\)", x)'),
    ],
}

def main():
    paths = [Path(a) for a in sys.argv[1:]] or sys.exit(__doc__.strip())
    files = []
    for p in paths:
        if p.is_file():
            files.append(p)
        elif p.is_dir():
            files.extend(f for f in p.rglob("*") if f.is_file())

    for suffix in _FILE_SWAPS:
        target = Path(suffix)
        if target.exists() and target not in files:
            files.append(target)

    for f in files:
        if f.name == "new_main.cpp":
            f.with_name("main.cpp").write_text(f.read_text())
            f.unlink()
            print(f"renamed {f}")
            continue
        if f.name == "CMakeLists.txt":
            old = f.read_text()
            new = (old
                   .replace("(BOOST_FIXTURE_TEST_SUITE|BOOST_AUTO_TEST_SUITE)", "TEST_SUITE_BEGIN")
                   .replace("--catch_system_error=no ", ""))
        elif f.suffix in (".cpp", ".h"):
            old = f.read_text()
            new = rewrite_macros(rewrite_includes(old))
        elif any(str(f).endswith(s) for s in _FILE_SWAPS):
            old = new = f.read_text()
        else:
            continue
        for suffix, swaps in _FILE_SWAPS.items():
            if str(f).endswith(suffix):
                for o, r in swaps:
                    if r is None:
                        new = re.sub(rf"^.*{re.escape(o)}.*\n", "", new, flags=re.MULTILINE)
                    else:
                        new = new.replace(o, r)
        if new != old:
            f.write_text(new)
            print(f"rewrote {f}")

if __name__ == "__main__":
    main()

</details>

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35587.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35594 (fuzz: cover async chainstate compaction by l0rinc)
• #35591 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
• #35590 (test: wallet: BnB incomplete result on attempt-limit success by brunoerg)
• #35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
• #35569 (Encapsulation for CTransaction by purpleKarrot)
• #35558 (p2p: Prefill compact blocks by davidgumberg)
• #35557 (kernel, validation: Add btck_chainstate_manager_set_clock_time by ryanofsky)
• #35531 (txindex: hash keys and pack positions to reduce disk usage by andrewtoth)
• #35511 (RFC: consensus: Make CAmount a class by hodlinator)
• #35501 (wallet: store all witness variants of a transaction by achow101)
• #35493 (wallet, descriptor: Fix MuSig private key completeness checks on importdescriptors by w0xlt)
• #35490 (test: cover unused mempool space in coins cache limit by w0xlt)
• #35474 (node: move index ownership to NodeContext by w0xlt)
• #35464 (kernel: Add function for creating chainparams with a signet challenge by sedited)
• #35461 (util: Clarify the assertion message in Assert, Assume and CHECK_NONFATAL by optout21)
• #35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)
• #35440 (wallet: check descriptor cache xpub length before decoding by alhudz)
• #35429 (wallet: avoid global access in external signer SPKM by w0xlt)
• #35422 (musig: Require generated secnonce for partial sig by nervana21)
• #35411 ([RFC] rpc: expose locked-memory arena_count from getmemoryinfo by HowHsu)
• #35406 (private broadcast: limit outstanding txs to count of 1000 by instagibbs)
• #35368 (tracing: add block header and compact block tracepoints by w0xlt)
• #35351 (net: Disallow invalid HeadersSyncState due to lagging clock by hodlinator)
• #35303 (policy: fix negative CFeeRate::ToString() formatting by joaonevess)
• #35302 (Silent Payments: Sending (take 2) by Eunovo)
• #35301 (Silent Payments: Implement bip352 (take 2) by Eunovo)
• #35295 (validation: fetch block input prevouts in parallel during ConnectBlock by andrewtoth)
• #35229 (refactor: Use CBlockIndex parameters as reference by optout21)
• #35225 (Policy: require minimal push in bare multisig output scripts by pinheadmz)
• #35215 (coins: use jumboblock SipHash-1-3 for hashing CCoinsMap keys by l0rinc)
• #35205 (kernel,node: clean up dbcache helpers and add kernel API by l0rinc)
• #35200 (node: smooth oversized dbcache warnings by l0rinc)
• #35195 (coins: cache UTXO outpoint hash codes by l0rinc)
• #35170 (test: Better test coverage for legacy ParseHDKeypath() by optout21)
• #35161 (consensus: document merkle mutation root invariant by l0rinc)
• #35139 (test: Add thread-safe fast-failing test macros by maflcko)
• #35084 (ipc: Add nonunix platform support by ryanofsky)
• #35069 (Refactor keypath parser by pythcoiner)
• #35027 (net: add -outboundbind option for outgoing source address by 8144225309)
• #35016 (net: deduplicate private broadcast state and snapshot types by takeshikurosawaa)
• #35003 (validation: improve block data I/O error handling in P2P paths by furszy)
• #34931 (validation: abort on DB unreadable coins instead of treating them as missing by furszy)
• #34875 (refactor: separate deferred script check collection from CheckInputScripts by l0rinc)
• #34872 (wallet: fix mixed-input transaction accounting in history RPCs by w0xlt)
• #34864 (coins: make cache freshness imply dirtiness and remove invalid test states by l0rinc)
• #34844 (util: Add util::NotNull<SmartPtrType> by maflcko)
• #34812 (net: advertise CJDNS addresses when -externalip disables discovery by w0xlt)
• #34778 (logging: rewrite macros to enforce restrictions at compile-time, improve efficiency and usability by ryanofsky)
• #34729 (Reduce log noise by ajtowns)
• #34707 (net: keep finished private broadcast txs in memory by andrewtoth)
• #34698 (wallet: handle MiniMiner bump fee calculation failures by shuv-amp)
• #34683 (rpc: support a formal description of our JSON-RPC interface by willcl-ark)
• #34672 (mining: add reason/debug to submitSolution and unify with submitBlock by w0xlt)
• #34538 (net: advertise -externalip addresses by willcl-ark)
• #34514 (refactor: remove unnecessary std::move for a few trivially copyable types by l0rinc)
• #34320 (coins: delegate CCoinsViewDB::HaveCoin to GetCoin by l0rinc)
• #34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
• #34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
• #34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)
• #33637 (refactor: optimize block index comparisons (1.4-6.8x faster) by l0rinc)
• #33540 (argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments) by pablomartin4btc)
• #33324 (blocks: add -reobfuscate-blocks argument to enable (de)obfuscating existing blocks by l0rinc)
• #32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
• #32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
• #32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
• #32575 (consensus: Remove special treatment for single threaded script checking by fjahr)
• #32427 (kernel: Replace leveldb-based BlockTreeDB with WAL and .dat file based store by sedited)
• #32317 (kernel: Separate UTXO set access from validation functions by sedited)
• #31682 ([IBD] specialize CheckBlock's input & coinbase checks by l0rinc)
• #29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
• #29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
• #29256 (log, refactor: Allow log macros to accept context arguments by ryanofsky)
• #29247 (CAT in Tapscript (BIP-347) by arminsabouri)
• #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
• #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Strong Concept ACK

My problems with boost

Boost may have multiple problems on its own, but the problems that you list are specific to Boost.Test. You should not conflate the two.

CHECK_EQUAL_RANGES(it1, it2) to compare two iterators

Iterators or Ranges?

TEST_SUITE_BEGIN/END to declare a suite

Boost.Test allows arbitrary deep nesting of test suites. When integrating Boost.Test into CTest, this causes headaches for little benefit. I hope we can follow-up on this and restrict the test hierarchy to exactly one test suite (no more and no less) using something along the TEST(TestSuiteName, TestName) macro from GoogleTest.

Why no concurrency?

In my opinion, concurrency does not belong in a testing framework. Many testing frameworks have a test driver built-in. But separating test framework and test driver into two orthogonal components allows much more powerful composition. I would even go so far and claim that a test framework should not provide a way to run more than one test; neither concurrently, nor sequentially.

What a test framework needs to be able to provide, is a way to launch a single test and a way to tell the test driver about the list of tests that are available. The test driver can then orchestrate tests from different testing frameworks from potentially different programming languages and invoke tests individually, sequentially, concurrently, or even distributed across multiple hosts.

This kind of integration is exactly the reason why I implemented discover_tests, which will be available in CMake 4.4.

The Boost.Test framework is notoriously bad at this. When selecting a single test, it spams out information about all tests that are not selected. While it can provide information about what tests are available, that output is optimized for human readability and very hard to parse by a test driver. We can definitely do better with a custom test framework!

There are a number of problems with using dependencies in general. Bugs must either be promptly upstreamed or patched, projects may be abandoned, and they are not tailored to a particular use case. bitcoind has a small number of dependencies and will have fewer very soon (https://github.com/bitcoin/bitcoin/pull/34411). One of the final remaining dependencies is boost, which is responsible for the unit tests and multi-index. This PR removes boost for writing and running unit tests.

Boost.Test is not a dependency of bitcoind, only of test_bitcoin so this is not comparable to the libevent removal. Also boost::multi_index removal was rejected very recently: #34586.

only of test_bitcoin

Correct me if I'm wrong but it appears the config here still builds test_bitcoin: https://github.com/bitcoin/bitcoin/blob/master/contrib/guix/libexec/build.sh#L125

Also boost::multi_index removal was rejected very recently: #34586

That approach used STL containers. An actual replacement would need to use something like this: https://www.mail-archive.com/digitalmars-d@puremagic.com/msg33920.html

Correct me if I'm wrong but it appears the config here still builds test_bitcoin

Yes, but what does that have to with my comment? I just said that it's a dependency of test_bitcoin not bitcoind. Reducing the dependencies of bitcoind is a lot more interesting than those of test_bitcoin for obvious security reasons. I don't think the fact that test_bitcoin is built in a release and installed changes much of that. Maybe it does for other people but I think your motivation should be more clear on this if this is what you are after. Particularly this part:

bitcoind has a small number of dependencies and will have fewer very soon (https://github.com/bitcoin/bitcoin/pull/34411). One of the final remaining dependencies is boost, which is responsible for the unit tests and multi-index.

I would read this very clearly as boost (including Boost.Test) is a dependency of bitcoind if I didn't know better.

That approach used STL containers.

The pull was rejected on conceptual basis, not approach, so I don't think this matters.

I would read this very clearly as boost (including Boost.Test) is a dependency of bitcoind if I didn't know better.

Sure, PR description updated.

Reducing the dependencies of bitcoind is a lot more interesting than those of test_bitcoin for obvious security reasons

I find this less obvious then the comment implies. A repository going stale is far more likely than some backdoor or high severity CVE, so "a lot more interesting" feels subjective.

Yes, but what does that have to with my comment?

A lot. @rustaceanrob correctly points out that even though Boost.Test is not strictly a dependency of bitcoind, it is included in the release flow that creates and packages bitcoind, along with test_bitcoin. It is correct to say that Boost.Test is a dependency of our release process.

A repository going stale is far more likely than some backdoor or high severity CVE, so "a lot more interesting" feels subjective.

Precisely. It's also worth remembering the xzutils backdoor was introduced through test code.

Strong Concept ACK!

Love to see it. While I broadly agree with the author's reasons stated in the PR, the ones I'll highlight specifically are:

• No repository context
• Not extensible

Having our own simple, in house test framework gives us the ability to customise it for exactly our needs. I believe we should do everything in our power to make testing an easy, intuitive, and feature rich experience as this helps to remove barriers to people writing and interpreting tests.

In particular, as a longer term project I am looking at ways to improve concurrency in our testing to help speed up CI. As @purpleKarrot points out:

The Boost.Test framework is notoriously bad at this. When selecting a single test, it spams out information about all tests that are not selected. While it can provide information about what tests are available, that output is optimized for human readability and very hard to parse by a test driver. We can definitely do better with a custom test framework!

996832947dd1053b2e31acd1ef3e2933804fd580 "test: Separate mock_process into a standalone binary"

This approach seems incompatible with the release process.

For example, I downloaded the bitcoin-e6e8d1f00a08-win64-unsigned.zip "release" binaries on a Windows machine, unpacked them, and ran:

>.\bitcoin-e6e8d1f00a08\libexec\test_bitcoin.exe
Running 770 test cases...
EXCEPTION in run_command: CreateProcess failed: The system cannot find the file specified.
[FAIL] run_command (0/1 checks failed)
[WARN]: Variable DIR_UNIT_TEST_DATA unset, skipping script_assets_test

770 tests: 769 passed, 1 failed, 0 skipped (26566129 checks)

This approach seems incompatible with the release process.

a0d535a78b63df4d04411efb1b2c0f98ee87211e uses an environment variable to determine if the test should run as the mock process or the actual test runner. No added files and should not effect the release.

Concept ACK 7974316a1a5e9e84be446f86fed52564ee954b4b

This change has enough pro's for me to support it;

• Unification of all the different BOOS_CHECK*
• Better orchestrate-able, see #35587 (comment)
• Removal of dependency.

The pull was rejected on conceptual basis, not approach, so I don't think this matters. @fjahr this is not an accurate framing of the discussion on #34586. It is acknowledged that fewer dependencies are desirable, if the replacements are an improvement. The critique that follows is specifically about the use of STL containers (which are indeed worse). The second push back from a different reviewer echos the sentiment: boost multi index is better than the alternatives (STL containers, in this case).

I think this PR stands on its own merit as better than using Boost.Test, regardless of whether or not we end up fully removing boost. However, if a better alternative to boost multi index were to be proposed (e.g., @rustaceanrob 's link), this PR and that perhaps future PR only strengthen each other, not weaken.