refactor: test: Unroll `&&` conditions in macros #35729

rustaceanrob commented at 12:40 PM on July 15, 2026: member

Picked from #35713. Given that I think this is a strict debugging improvement, I opened as a separate pull:

Using && in BOOST_CHECK is problematic as failures will not indicate which condition failed. By unrolling these checks, the user knows exactly which expression is the failing case.

As an example, here is a line that would be particularly hard to debug if it failed:

src/test/net_tests.cpp

BOOST_CHECK((*ret)[1] && (*ret)[1]->m_type == "headers" && std::ranges::equal((*ret)[1]->m_recv, MakeByteSpan(msg_data_2)));

If any one of these conditions fail, the whole expression fails, with no values printed or indication as to which condition failed.

This is also required when using test macros that support value decomposition, which requires && and || are delete. Examples include BOOST_TEST, doctest, Catch2, etc.

ref: https://catch2-temp.readthedocs.io/en/latest/assertions.html#other-limitations ref: https://fekir.info/post/decomposing-an-expression/

DrahtBot added the label Refactoring on Jul 15, 2026

DrahtBot commented at 12:40 PM on July 15, 2026: contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	maflcko, ismaelsadeeq

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35713 (Remove boost as a unit test runner by rustaceanrob)
#34864 (coins: tighten cache entry state invariants by l0rinc)
#33540 (argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments) by pablomartin4btc)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file 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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

DecodeBase58(base58string, result, 256) in src/test/base58_tests.cpp
Test(ms_stack_limit, "?", "?", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_NEEDSIG | TESTMODE_P2WSH_INVALID, 4 * count + 1, 1, {}, {}, 1 + count + 1) in src/test/miniscript_tests.cpp
Test(ms_stack_nok, "?", "?", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_NEEDSIG | TESTMODE_P2WSH_INVALID, 4 * count + 1, 1, {}, {}, 1 + count + 1) in src/test/miniscript_tests.cpp
Lookup("2a10:1234:5678:9abc:def0:1234:5678:9abc", 1, false) in src/test/pcp_tests.cpp
Lookup("2a10:1234:5678:9abc:def0:0000:0000:0000", 1, false) in src/test/pcp_tests.cpp
Lookup("192.168.0.1", 1, false) in src/test/pcp_tests.cpp
Lookup("2a10:1234:5678:9abc:def0:1234:5678:9abc", 1, false) in src/test/pcp_tests.cpp
builder.Add(2, script_2, 0xc0) in src/test/script_standard_tests.cpp

in src/test/pcp_tests.cpp:55 in 5a85298c2d outdated

  48 | @@ -49,7 +49,10 @@ class PCPTestingSetup : public BasicTestingSetup
  49 |          const std::optional<CService> local_ipv6{Lookup("2a10:1234:5678:9abc:def0:1234:5678:9abc", 1, false)};
  50 |          const std::optional<CService> gateway_ipv4{Lookup("192.168.0.1", 1, false)};
  51 |          const std::optional<CService> gateway_ipv6{Lookup("2a10:1234:5678:9abc:def0:0000:0000:0000", 1, false)};
  52 | -        BOOST_REQUIRE(local_ipv4 && local_ipv6 && gateway_ipv4 && gateway_ipv6);
  53 | +        BOOST_REQUIRE(local_ipv4);
  54 | +        BOOST_REQUIRE(local_ipv6);
  55 | +        BOOST_REQUIRE(gateway_ipv4);
  56 | +        BOOST_REQUIRE(gateway_ipv6);

maflcko commented at 1:00 PM on July 15, 2026:

seems fine, but I wonder what the difference is between BOOST_REQUIRE and Assert.

There are already a few tests that use Assert:

git grep 'Assert(' ./src/test/*.cpp

Also, It is shorter to write either:

        const CService gateway_ipv6{*Assert(Lookup("2a10:1234:5678:9abc:def0:0000:0000:0000", 1, false))};

or:

        default_local_ipv6 = *Assert(local_ipv6);

than to use the BOOST_REQUIRE.

So my preference would be to move toward Assert where it makes sense, to avoid bloating the tests, but this seems also acceptable as-is.

rustaceanrob commented at 1:26 PM on July 15, 2026:

I count around ~36 BOOST_REQUIRE in this patch. I like how this is reviewable by just reading the added lines do exactly what the red line did, only in separate conditions, but if there is a strong preference for Assert I can do that here.

maflcko commented at 1:36 PM on July 15, 2026:

Sure, seems fine to leave as-is. Just leaving a few comments, because if this was BOOST_CHECK (like in a few other places, see the comments), it would be UB.

in src/test/argsman_tests.cpp:411 in 5a85298c2d outdated

 406 | @@ -397,8 +407,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
 407 |      // Command line overrides, but doesn't erase old setting
 408 |      BOOST_CHECK(!testArgs.IsArgNegated("-bar"));
 409 |      BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "");
 410 | -    BOOST_CHECK(testArgs.GetArgs("-bar").size() == 1
 411 | -                && testArgs.GetArgs("-bar").front() == "");
 412 | +    BOOST_CHECK(testArgs.GetArgs("-bar").size() == 1);
 413 | +    BOOST_CHECK(testArgs.GetArgs("-bar").front() == "");

maflcko commented at 1:32 PM on July 15, 2026:

nit: front() could be UB. Not sure if we care about that, because there is a bunch of more UB in tests.

rustaceanrob commented at 3:09 PM on July 15, 2026:

I think one fix here would be to change the above check to BOOST_REQUIRE, but maybe in these cases I will migrate them to Assert?

rustaceanrob commented at 11:24 AM on July 16, 2026:

I changed the some sites to BOOST_REQUIRE to stop the test before potential UB. Avoided Assert to not add more includes because it would be noisy for review.

in src/test/base58_tests.cpp:62 in 5a85298c2d outdated

  57 | @@ -58,7 +58,8 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
  58 |          std::vector<unsigned char> expected = ParseHex(test[0].get_str());
  59 |          std::string base58string = test[1].get_str();
  60 |          BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
  61 | -        BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
  62 | +        BOOST_CHECK_MESSAGE(result.size() == expected.size(), strTest);
  63 | +        BOOST_CHECK_MESSAGE(std::equal(result.begin(), result.end(), expected.begin()), strTest);

maflcko commented at 1:33 PM on July 15, 2026:

Could be one line shorter with ranges::equal?

ismaelsadeeq commented at 2:00 PM on July 16, 2026:

Yeah, this is also a potential UB, it seems the ranges::equal fixed this.

in src/test/miniscript_tests.cpp:640 in 5a85298c2d outdated

 635 | @@ -636,13 +636,15 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
 636 |      ms_stack_limit += "pk(" + HexStr(g_testdata->pubkeys[0]) + ")";
 637 |      ms_stack_limit.insert(ms_stack_limit.end(), count, ')');
 638 |      const auto ms_stack_ok{miniscript::FromString(ms_stack_limit, tap_converter)};
 639 | -    BOOST_CHECK(ms_stack_ok && ms_stack_ok->CheckStackSize());
 640 | +    BOOST_CHECK(ms_stack_ok);
 641 | +    BOOST_CHECK(ms_stack_ok->CheckStackSize());

maflcko commented at 1:34 PM on July 15, 2026:

nit: UB here as well

maflcko approved

maflcko commented at 1:37 PM on July 15, 2026: member

lgtm. There is a slight sprinkle of UB, but I don't think it matters.

refactor: test: Unroll `&&` conditions in macros

Using `&&` in `BOOST_CHECK` is problematic as failures will not indicate
which condition failed. By unrolling these checks, the user knows
exactly which expression is the failing case.

As an example, here is a line that would be particularly hard to debug
if it failed:

```
src/test/net_tests.cpp

BOOST_CHECK((*ret)[1] && (*ret)[1]->m_type == "headers" && std::ranges::equal((*ret)[1]->m_recv, MakeByteSpan(msg_data_2)));
```

If any one of these conditions fail, the whole expression fails, with no
values printed or indication as to which condition failed.

This is also required when using test macros that support value
decomposition, which requires `&&` and `||` are `delete`. Examples
include `BOOST_TEST`, doctest, Catch2, etc.

ref: https://catch2-temp.readthedocs.io/en/latest/assertions.html#other-limitations
ref: https://fekir.info/post/decomposing-an-expression/

490327bf9d

rustaceanrob force-pushed on Jul 16, 2026

maflcko commented at 12:04 PM on July 16, 2026: member

review ACK 490327bf9da4c4ee47ed238b748ca94376d9c94d 🏜

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 490327bf9da4c4ee47ed238b748ca94376d9c94d 🏜
MihS3h2h8vren+QfYkzouXQDeHzKRPODOkFn0Tjl6vxOL3kLZXorHd5dE5Pee8hdehXJAv68rVLNWmi0mGxZAg==

</details>

ismaelsadeeq approved

ismaelsadeeq commented at 2:13 PM on July 16, 2026: member

ACK 490327bf9da4c4ee47ed238b748ca94376d9c94d

It is worth noting that breaking the chaining checks has behaviour changes, There are instances where the execution of invariants depends on some being succeeding; that's why we are seeing the U.B's. I see some are guarded using require.

I don't know how much we worry about that, but I did a quick scan and didn't find more apart from ones spotted before.

If we are going to do this and make this a rule, maybe add a lint job that may see and warn against chaining, as is now, nothing will prevent adding the chaining now. You may either have to review all PR before they are merged or do some unrolling again later.

maflcko commented at 3:00 PM on July 16, 2026: member

or do some unrolling again later.

I'd say it should be rare enough to do another unroll later, if there is need to.

in src/test/coinscachepair_tests.cpp:25 in 490327bf9d

  20 | @@ -21,7 +21,8 @@ std::list<CoinsCachePair> CreatePairs(CoinsCachePair& sentinel)
  21 |          auto node{std::prev(nodes.end())};
  22 |          CCoinsCacheEntry::SetDirty(*node, sentinel);
  23 |  
  24 | -        BOOST_CHECK(node->second.IsDirty() && !node->second.IsFresh());
  25 | +        BOOST_CHECK(node->second.IsDirty());
  26 | +        BOOST_CHECK(!node->second.IsFresh());

l0rinc commented at 4:57 PM on July 16, 2026:

The previous version made more sense since fresh implies dirty

rustaceanrob commented at 12:24 PM on July 17, 2026:

I'm not sure what you mean by "made more sense" because both checks are equivalent in this diff. If you are implying there is an invariant that should fail the test immediately, the second check can be converted to a REQUIRE if you prefer.

l0rinc commented at 5:00 PM on July 16, 2026: contributor

I'm not going to block the PR, but I disagree with many examples here - and it invalidates a lot of other PRs. Some of the conditions make sense in groups - the author of the change should decide that, this just seems a more noisy way to express the same. Sure, on failure it may make debugging easier, but it can also make the code harder to read by pushing everything to the foreground.

maflcko commented at 5:39 AM on July 17, 2026: member

Some of the conditions make sense in groups

The options:

Give up on the idea of the single TEST macro
Group the groups explicitly with double-braces: TEST( ( a && b ) )
Something else?

rustaceanrob commented at 12:21 PM on July 17, 2026: member

Sure, on failure it may make debugging easier

This is exactly the point of the pull. Why would we waste developer time by having them find which of the N conditions failed when it is logically equivalent to N independent checks? Saving time debugging should be the default, and if you insist on omitting the useful debug output and grouping conditions together, you may wrap the && conditions in ( ... ) as was suggested above.

can also make the code harder to read

This is subjective. Personally I find:

 BOOST_CHECK(s.command_line_options.contains("a") && s.command_line_options.contains("b") && s.command_line_options.contains("ccc")
                    && !s.command_line_options.contains("f") && !s.command_line_options.contains("d"));

difficult to read.

maflcko commented at 12:31 PM on July 17, 2026: member

Some of the conditions make sense in groups

The options:

* Give up on the idea of the single `TEST` macro

* Group the groups explicitly with double-braces: `TEST( ( a && b ) )`

* Something else?

Just ran into this myself, but for another reason: The stringify isn't provided for some types.

In that case (or similar cases), where the dev really only cares about a boolean outcome, as a third alternative, the outcome could be clearly assigned to a bool with a descriptive name. E.g.:

    util::NotNull p{std::make_shared<MyDerived>()};
    util::NotNull q{std::make_shared<MyBase>()};
    q = p;
    const bool same_ptr{q == p};
    BOOST_CHECK(same_ptr);
    // doesn't compile:
    //BOOST_CHECK_EQUAL(q,p);
    //BOOST_TEST(q==p);

l0rinc commented at 7:03 PM on July 17, 2026: contributor

Why would we waste developer time by having them find which of the N conditions failed

Sure, but the opposite also applies: why would we waste developer time by separating groups of conditions? The S/N ratio matters for readability, and when conditions are logically connected it can make sense to represent them as such in code.

Personally I find: BOOST_CHECK(s.command_line_options.contains("a") && s.command_line_options.contains("b") && s.command_line_options.contains("ccc") && !s.command_line_options.contains("f") && !s.command_line_options.contains("d")); difficult to read.

Yes, this definitely needs to go (e.g. a for loop could fix it), but this isn't the example I gave, but:

BOOST_CHECK(node->second.IsDirty() && !node->second.IsFresh());

and

BOOST_CHECK(flushed && state.IsValid());

where most people just skipped state validation since adding yet another assertion line seemed too noisy.

you may wrap the && conditions in ( ... ) as was suggested above.

Yes, or immediately invoked lambda expressions probably work also.

rustaceanrob commented at 5:12 PM on July 21, 2026: member

why would we waste developer time by separating groups of conditions?

Tests are written by a single developer whereas all developers run the tests, so this characterization is not an accurate analogy. If a test fails in an unexpected way, the developer will be focused on debugging, not test legibility. In fact, for the example you gave:

BOOST_CHECK(flushed && state.IsValid());

Suppose this condition failed. My immediate reaction would be to separate these two checks to find which of the conditions are failing, then possibly running with gdb after. You did not respond to the comment above, which suggested to rewrite these types of groupings as:

BOOST_CHECK(flushed);
BOOST_REQUIRE(state.IsValid());

This clearly expresses the invariant. If flushed was okay, the state must be okay, else something critically wrong has happened and we may fail the test immediately.

Furthermore, Catch2, doctest, BOOST_TEST all do not support && conditions due to operator precedence. This tradeoff was deemed worthwhile, and is used by every modern framework. You can verify this by applying this diff to your pull request:

diff --git a/src/test/chainstate_write_tests.cpp b/src/test/chainstate_write_tests.cpp
index e211a0456a..cafaabd57e 100644
--- a/src/test/chainstate_write_tests.cpp
+++ b/src/test/chainstate_write_tests.cpp
@@ -125,7 +125,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_flush_failure_boundary, TestChain100Setup)

     const bool flushed{chainstate.FlushStateToDisk(state, FlushStateMode::FORCE_FLUSH)};
     BOOST_CHECK_EQUAL(m_node.exit_status.load(), EXIT_FAILURE);
-    BOOST_CHECK(!flushed && state.IsError());
+    BOOST_TEST(!flushed && state.IsError());
     BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return chainstate.GetLastFlushedBlock()), old_flushed);
 }

At which point you run into this failure:

/home/rob/bitcoin-core/review/src/test/chainstate_write_tests.cpp:128:25: note: in instantiation of function template specialization 'boost::test_tools::assertion::expression_base<boost::test_tools::assertion::value_expr<bool>, bool>::operator&&<bool>' requested here
  128 |     BOOST_TEST(!flushed && state.IsError());
      |                         ^
include/boost/mpl/assert.hpp:83:5: note: candidate function template not viable: no known conversion from 'boost::mpl::failed ************(CANT_USE_LOGICAL_OPERATOR_AND_WITHIN_THIS_TESTING_TOOL::************)()' to 'typename assert<false>::type' (aka 'mpl_::assert<false>') for 1st argument
   83 | int assertion_failed( typename assert<C>::type );
      |     ^                 ~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
[432/544] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/descriptor_tests.cpp.o
ninja: build stopped: subcommand failed.

With the verbose CANT_USE_LOGICAL_OPERATOR_AND_WITHIN_THIS_TESTING_TOOL diagnostic.

This pull request is not preventing the style you propose either, if the debug output is undesired, as you may always wrap the conditions. This compiles fine:

-    BOOST_CHECK(!flushed && state.IsError());
+    BOOST_TEST((!flushed && state.IsError()));

l0rinc commented at 6:23 PM on July 21, 2026: contributor

Suppose this condition failed

If this failed, it's because of something the developer just changed. They can just unroll locally, rerun the test and check - like you mentioned, it's trivial, takes 10 seconds - we do it all the time. Outside of that narrow window everyone else has to parse several lines if we split, so the minority use case (failure caused by my local change) is prioritized over the majority case (people running passing tests and reading code).

This pull request is not preventing the style you propose either

Yes, so let's split only the conditions like you mentioned, which obviously shouldn't be in the same line - without touching conflicting lines that invalidate other long-standing PRs, just to make something subjectively pretty in a theoretical and unlikely case. Ping me for a review if you do that, but in its current form I'm not enthusiastic about this change. I won't block it, but I don't like that we're narrowing the ways we can express ourselves to have the tests reflect the intent.