test: Add debug output to common tested types #36091

rustaceanrob commented at 11:54 AM on August 26, 2026: member

If the << operator exists, boost can use this as a debug output when a check fails (besides BOOST_CHECK). This adds << for types commonly used across the unit tests that do not yet have a display output. Each callsite that uses these types are then updated to use macros that will print the debug output on failure. All of these display strings would be used as part of #35713, but we can also use them with boost today.

DrahtBot added the label Tests on Aug 26, 2026

DrahtBot commented at 11:54 AM on August 26, 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/36091.

Reviews

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

Type	Reviewers
ACK	ismaelsadeeq, jeanpablojp
Concept ACK	sedited, brunoerg

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:

#36122 (BIP460: CISA for Taproot key path spends by fjahr)
#35713 (Remove boost as a unit test runner by rustaceanrob)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#35139 (test: Add thread-safe fast-failing test macros by maflcko)
#35003 (validation: improve block data I/O error handling in P2P paths by furszy)

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.

sedited commented at 12:07 PM on August 26, 2026: contributor

Concept ACK

brunoerg commented at 1:10 PM on August 26, 2026: contributor

Concept ACK

DrahtBot added the label CI failed on Aug 26, 2026

DrahtBot commented at 1:32 PM on August 26, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/32965723886/job/98167644980</sub> <sub>LLM reason (✨ experimental): CI failed because the fuzz test target rpc crashed (exit code 3221225477, Windows access violation).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Aug 26, 2026

jeanpablojp commented at 3:07 PM on August 29, 2026: contributor

Concept ACK

Left three comments.

in src/test/util/display.h:19 in 92d7a74c0b outdated

  14 | +#include <protocol.h>
  15 | +#include <pubkey.h>
  16 | +#include <script/keyorigin.h>
  17 | +#include <tinyformat.h>
  18 | +#include <util/bip32.h>
  19 | +#include <util/feefrac.h>

jeanpablojp commented at 3:07 PM on August 29, 2026:

feefrac_tests and txgraph_tests only need the FeeFrac printers, but this block drags addrman, key_io and protocol in with it. Touching addrman now rebuilds both, and it's about 12k extra preprocessed lines in the first one. Would it be worth splitting this by area?

rustaceanrob commented at 9:28 AM on August 30, 2026:

In the current state of #35713 I have it like this; however, I'm a little hesitant to make this change because having each test translation unit define the << would bloat the tests with additional code that doesn't pertain to the logic of the test. With everything defined in a single file, developers know where to make changes to the debug string, but otherwise it is out of the way. I can adjust if other reviewers agree with this suggestion.

maflcko commented at 2:24 PM on September 1, 2026:

Most of the functions can be just the signature and a fwd decl for the type, and the actual stringify can move to a cpp file?

This saves re-compilation at least in the other cases (AddressPosition, etc)

Also, if this is the new display module, the other stuff should move here from ./src/test/util/common.h later?

rustaceanrob commented at 2:40 PM on September 1, 2026:

Also, if this is the new display module, the other stuff should move here from ./src/test/util/common.h later?

This could just as well go into common, I was just trying to avoid bloat in that file bc I wasn't sure if debug outputs are "common", but I suppose they are.

Most of the functions can be just the signature and a fwd decl for the type, and the actual stringify can move to a cpp file?

This saves re-compilation at least in the other cases (AddressPosition, etc)

Sure, we can go this route. I don't have a strong preference.

rustaceanrob commented at 3:25 PM on September 1, 2026:

Moved the definitions to display.cpp. Can figure out the common.h situation later IMO.

maflcko commented at 9:52 AM on September 2, 2026:

Can figure out the common.h situation later IMO.

It probably makes sense for HasReason to sit in the same header/module? If all display stuff is grouped with one include, it would seem odd to require another include for HasReason. But no strong opinion. Also, not sure how if this will change with #35713 and the display+HasReason stuff is provided by the framework include?

rustaceanrob commented at 10:09 AM on September 2, 2026:

I intend to invert where the display comes from so framework imports common. Since common and the header here are serving the same purpose, I think I will add these forward decl to common.h and make a common.cpp here.

rustaceanrob commented at 2:41 PM on September 2, 2026:

Added common.cpp with the implementations and all forward delc to common.h in latest push

in src/test/util/display.h:35 in 92d7a74c0b outdated

  30 | +inline std::ostream& operator<<(std::ostream& os, const CService& service)
  31 | +{
  32 | +    return os << service.ToStringAddrPort();
  33 | +}
  34 | +
  35 | +inline std::ostream& operator<<(std::ostream& os, const CAddress& addr)

jeanpablojp commented at 3:07 PM on August 29, 2026:

This one leaves out nTime and nServices, which operator== does compare, so in the two addrv1/addrv2 unserialize checks a regression in time or in flags comes out as [::1]:0 != [::1]:0. The CTxDestination one prints CTxDestination() for CNoDestination and PubKeyDestination. Worth checking each printer against the type's operator==?

rustaceanrob commented at 9:20 AM on August 30, 2026:

Added additional info for CAddress and took a glance through the rest of the ==, I think all fields should be covered. As far as CTxDestination, I added specific displays for each variant and the CTxDestination printer now just passes through to those.

jeanpablojp commented at 12:02 PM on August 30, 2026:

While checking the changes I noticed a small nit. CService's == compares m_net, which the printer doesn't show, so IPv6 and CJDNS with the same bytes come out identical. MaybeFlipIPv6toCJDNS in advertise_local_address builds the pair. No check reaches that today.

rustaceanrob commented at 11:04 AM on August 31, 2026:

Added a net field to CService

in src/test/util/display.h:45 in 92d7a74c0b outdated

  40 | +inline std::ostream& operator<<(std::ostream& os, const CTxDestination& dest)
  41 | +{
  42 | +    return os << strprintf("CTxDestination(%s)", EncodeDestination(dest));
  43 | +}
  44 | +
  45 | +inline std::ostream& operator<<(std::ostream& os, const CExtKey& k)

jeanpablojp commented at 3:07 PM on August 29, 2026:

Encode() asserts on an empty key, which is what DecodeExtKey leaves you with when it fails, and that's the value bip32_tests compares. I broke the decode to see. The case dies on SIGABRT and takes the following ones with it. On master the same break just prints 17 failures and the suite finishes. Worth a guard here and in the CExtPubKey one?

rustaceanrob commented at 9:18 AM on August 30, 2026:

If I understood this correctly I added a length guard for each of these. Lmk if that was what you were suggesting.

jeanpablojp commented at 12:03 PM on August 30, 2026:

Yes, that's what I meant. I broke the decode again and now it prints CExtKey(<invalid>) and the suite finishes, no SIGABRT. Small thing, the CExtPubKey invalid branch says CPubExtKey.

rustaceanrob commented at 11:02 AM on August 31, 2026:

Fixed thanks

rustaceanrob force-pushed on Aug 30, 2026

rustaceanrob force-pushed on Aug 31, 2026

rustaceanrob force-pushed on Sep 1, 2026

in src/test/util/display.h:51 in 7efdb17af6

  46 | +
  47 | +template <std::derived_from<FeeFrac> T>
  48 | +inline std::ostream& operator<<(std::ostream& os, const T& ff)
  49 | +{
  50 | +    return os << strprintf("FeeFrac(fee=%d, size=%d)", ff.fee, ff.size);
  51 | +}

purpleKarrot commented at 4:11 PM on September 1, 2026:

No need for separate template instantiations for each derived type:

inline std::ostream& operator<<(std::ostream& os, const FeeFrac& ff)
{
    return os << strprintf("FeeFrac(fee=%d, size=%d)", ff.fee, ff.size);
}

And then you can also move the implementation to the source file forward declare FeeFrac in the header.

rustaceanrob commented at 4:48 PM on September 1, 2026:

I de-templated ByRatioNegSize as well because the only intended and current use case is with FeeFrac. Updated in latest push.

rustaceanrob force-pushed on Sep 1, 2026

in src/test/util/display.h:40 in 7cc37297cf

  35 | +std::ostream& operator<<(std::ostream& os, const PayToAnchor& p);
  36 | +std::ostream& operator<<(std::ostream& os, const WitnessV1Taproot& t);
  37 | +std::ostream& operator<<(std::ostream& os, const CTxDestination& dest);
  38 | +std::ostream& operator<<(std::ostream& os, const CPubKey& pk);
  39 | +std::ostream& operator<<(std::ostream& os, const KeyOriginInfo& info);
  40 | +std::ostream& operator<<(std::ostream& os, const std::pair<CPubKey, KeyOriginInfo>& p);

maflcko commented at 5:30 PM on September 1, 2026:

Why is this pair thing needed? Seems to be a single test-only thing? So either this should be moved to that test, or maybe pair should be handled generally, like the existing optional:

template <typename T>
inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
{
    return v ? os << *v
             : os << "std::nullopt";
}

Either would allow dropping the includes for this.

rustaceanrob commented at 9:23 AM on September 2, 2026:

There is a callsite that uses both BOOST_CHECK and std::pair on types that do not implement <<. #35713 works around this because both types make sense as hex representations, but I couldn't compile after rebasing on this branch because operator<< resolved first. I added a requires requires(std::ostream& os, const T& t, const U& u) { os << t; os << u; }, which I think it correct. Otherwise yeah this should be general, updated in latest push.

maflcko commented at 9:49 AM on September 2, 2026:

Ah interesting. I wonder if the hex fallback should be added here (even if unused). The benefit would be that the stringify logic and fallback logic would only need to be reviewed once.

rustaceanrob commented at 10:02 AM on September 2, 2026:

I am currently using typeid as additional information in that debug output, but that doesn't seem great since it'll be a mangled type name. Another option is to just add HexType(0x00..00). Either way, seems like that can occur in a different pull since this one is to improve specific callsites?

rustaceanrob force-pushed on Sep 1, 2026

maflcko approved

maflcko commented at 10:14 AM on September 2, 2026: member

approach lgtm.

Haven't looked closely, but all of this seems fine.

rustaceanrob force-pushed on Sep 2, 2026

in src/test/util/common.cpp:121 in 05bd1513be

 116 | +    return os;
 117 | +}
 118 | +
 119 | +std::ostream& operator<<(std::ostream& os, const CPubKey& pk)
 120 | +{
 121 | +    return os << strprintf("CPubKey(%s)", HexStr(pk));

ismaelsadeeq commented at 6:53 PM on September 7, 2026:

In "test: Add display operator<< overloads to common.{h,cpp}" 05bd1513be2d9aaeb0a94671024f585f0d3cca15

nit: add context to what we are displaying here and other places.

diff --git a/src/test/util/common.cpp b/src/test/util/common.cpp
index ea9b42525a..6f0229c21e 100644
--- a/src/test/util/common.cpp
+++ b/src/test/util/common.cpp
@@ -44,7 +44,7 @@ std::ostream& operator<<(std::ostream& os, const CExtKey& k)
     if (!k.key.IsValid()) return os << "CExtKey(<invalid>)";
     unsigned char code[BIP32_EXTKEY_SIZE];
     k.Encode(code);
-    return os << strprintf("CExtKey(%s)", HexStr(code));
+    return os << strprintf("CExtKey(encoded=%s)", HexStr(code));
 }

 std::ostream& operator<<(std::ostream& os, const CExtPubKey& k)
@@ -52,7 +52,7 @@ std::ostream& operator<<(std::ostream& os, const CExtPubKey& k)
     if (k.pubkey.size() != CPubKey::COMPRESSED_SIZE) return os << "CExtPubKey(<invalid>)";
     unsigned char code[BIP32_EXTKEY_SIZE];
     k.Encode(code);
-    return os << strprintf("CExtPubKey(%s)", HexStr(code));
+    return os << strprintf("CExtPubKey(encoded=%s)", HexStr(code));
 }

 std::ostream& operator<<(std::ostream& os, const FeeFrac& ff)
@@ -67,32 +67,32 @@ std::ostream& operator<<(std::ostream& os, const ByRatioNegSize<FeeFrac>& b)

 std::ostream& operator<<(std::ostream& os, const CNoDestination& dest)
 {
-    return os << strprintf("CNoDestination(%s)", HexStr(dest.GetScript()));
+    return os << strprintf("CNoDestination(script=%s)", HexStr(dest.GetScript()));
 }

 std::ostream& operator<<(std::ostream& os, const PubKeyDestination& dest)
 {
-    return os << strprintf("PubKeyDestination(%s)", HexStr(dest.GetPubKey()));
+    return os << strprintf("PubKeyDestination(pubkey=%s)", HexStr(dest.GetPubKey()));
 }

 std::ostream& operator<<(std::ostream& os, const PKHash& h)
 {
-    return os << strprintf("PKHash(%s)", h.ToString());
+    return os << strprintf("PKHash(hash=%s)", h.ToString());
 }

 std::ostream& operator<<(std::ostream& os, const ScriptHash& h)
 {
-    return os << strprintf("ScriptHash(%s)", h.ToString());
+    return os << strprintf("ScriptHash(hash=%s)", h.ToString());
 }

 std::ostream& operator<<(std::ostream& os, const WitnessV0KeyHash& h)
 {
-    return os << strprintf("WitnessV0KeyHash(%s)", h.ToString());
+    return os << strprintf("WitnessV0KeyHash(hash=%s)", h.ToString());
 }

 std::ostream& operator<<(std::ostream& os, const WitnessV0ScriptHash& h)
 {
-    return os << strprintf("WitnessV0ScriptHash(%s)", h.ToString());
+    return os << strprintf("WitnessV0ScriptHash(hash=%s)", h.ToString());
 }

 std::ostream& operator<<(std::ostream& os, const WitnessUnknown& w)
@@ -107,7 +107,7 @@ std::ostream& operator<<(std::ostream& os, const PayToAnchor& p)

 std::ostream& operator<<(std::ostream& os, const WitnessV1Taproot& t)
 {
-    return os << strprintf("WitnessV1Taproot(%s)", HexStr(t));
+    return os << strprintf("WitnessV1Taproot(program=%s)", HexStr(t));
 }

 std::ostream& operator<<(std::ostream& os, const CTxDestination& dest)
@@ -118,7 +118,7 @@ std::ostream& operator<<(std::ostream& os, const CTxDestination& dest)

 std::ostream& operator<<(std::ostream& os, const CPubKey& pk)
 {
-    return os << strprintf("CPubKey(%s)", HexStr(pk));
+    return os << strprintf("CPubKey(pubkey=%s)", HexStr(pk));
 }

 std::ostream& operator<<(std::ostream& os, const KeyOriginInfo& info)

</details>

rustaceanrob commented at 9:25 AM on September 8, 2026:

Makes sense. Applied those changes as well as adding a addr field to CService and CAddress.

in src/test/bip32_tests.cpp:11 in 056bd2582b outdated

   7 | @@ -8,6 +8,7 @@
   8 |  #include <key.h>
   9 |  #include <key_io.h>
  10 |  #include <streams.h>
  11 | +#include <test/util/common.h>

ismaelsadeeq commented at 7:05 PM on September 7, 2026:

In "test: Convert BOOST_CHECK macros with known debug string" 056bd2582bc96f1fc2b47933badcd4fe85832c3c

nit: It will be nice if callers don't have to include this in order to use the displayable macros. Reviewers now have to point to pr authors so they can use it instead of boost check. Because when u use boost check equal without the include, it won't compile.

Should we make common.h self-advertising so authors discover it without a reviewer? A short header comment listing the types it makes BOOST_CHECK_*-printable, e.g.:

// Provides operator<< for these types so they can be used with BOOST_CHECK_EQUAL/_NE:
//   AddressPosition, CService, CAddress, CExtKey/CExtPubKey, FeeFrac, CTxDestination
//   (+ all its alternatives), CPubKey, KeyOriginInfo, BlockTxPosition, std::pair, ...
// Include this header in your *_tests.cpp to print values on failure.

rustaceanrob commented at 10:50 AM on September 8, 2026:

Added a comment to common.h describing the purpose of the file. I think it would be a bit cumbersome to update the list on each addition to this file, and the developer would already be either in the file or one click away, so I noted that debug-available types are listed in the header.

in src/test/bip32_tests.cpp:139 in 056bd2582b outdated

 135 | @@ -135,11 +136,11 @@ void RunTest(const TestVector& test)
 136 |  
 137 |          // Test private key
 138 |          BOOST_CHECK(EncodeExtKey(key) == derive.prv);
 139 | -        BOOST_CHECK(DecodeExtKey(derive.prv) == key); //ensure a base58 decoded key also matches
 140 | +        BOOST_CHECK_EQUAL(DecodeExtKey(derive.prv), key); //ensure a base58 decoded key also matches

ismaelsadeeq commented at 7:07 PM on September 7, 2026:

In "test: Convert BOOST_CHECK macros with known debug string" 056bd2582bc96f1fc2b47933badcd4fe85832c3c

It seems you missed some.

diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp
index 5022e1f7b4..9c87ba7c91 100644
--- a/src/test/script_standard_tests.cpp
+++ b/src/test/script_standard_tests.cpp
@@ -242,14 +242,14 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination)
     s.clear();
     s << OP_DUP << OP_HASH160 << ToByteVector(pubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG;
     BOOST_CHECK(ExtractDestination(s, address));
-    BOOST_CHECK(std::get<PKHash>(address) == PKHash(pubkey));
+    BOOST_CHECK_EQUAL(std::get<PKHash>(address), PKHash(pubkey));

     // TxoutType::SCRIPTHASH
     CScript redeemScript(s); // initialize with leftover P2PKH script
     s.clear();
     s << OP_HASH160 << ToByteVector(CScriptID(redeemScript)) << OP_EQUAL;
     BOOST_CHECK(ExtractDestination(s, address));
-    BOOST_CHECK(std::get<ScriptHash>(address) == ScriptHash(redeemScript));
+    BOOST_CHECK_EQUAL(std::get<ScriptHash>(address), ScriptHash(redeemScript));

     // TxoutType::MULTISIG
     s.clear();
@@ -267,7 +267,7 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination)
     BOOST_CHECK(ExtractDestination(s, address));
     WitnessV0KeyHash keyhash;
     CHash160().Write(pubkey).Finalize(keyhash);
-    BOOST_CHECK(std::get<WitnessV0KeyHash>(address) == keyhash);
+    BOOST_CHECK_EQUAL(std::get<WitnessV0KeyHash>(address), keyhash);

     // TxoutType::WITNESS_V0_SCRIPTHASH
     s.clear();
@@ -275,14 +275,14 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination)
     CSHA256().Write(redeemScript.data(), redeemScript.size()).Finalize(scripthash.begin());
     s << OP_0 << ToByteVector(scripthash);
     BOOST_CHECK(ExtractDestination(s, address));
-    BOOST_CHECK(std::get<WitnessV0ScriptHash>(address) == scripthash);
+    BOOST_CHECK_EQUAL(std::get<WitnessV0ScriptHash>(address), scripthash);

     // TxoutType::WITNESS_V1_TAPROOT
     s.clear();
     auto xpk = XOnlyPubKey(pubkey);
     s << OP_1 << ToByteVector(xpk);
     BOOST_CHECK(ExtractDestination(s, address));
-    BOOST_CHECK(std::get<WitnessV1Taproot>(address) == WitnessV1Taproot(xpk));
+    BOOST_CHECK_EQUAL(std::get<WitnessV1Taproot>(address), WitnessV1Taproot(xpk));

     // TxoutType::ANCHOR
     s.clear();

</details>

rustaceanrob commented at 9:25 AM on September 8, 2026:

Applied those changes, thanks

in src/test/util/common.h:94 in 05bd1513be outdated

  89 | +std::ostream& operator<<(std::ostream& os, const WitnessV1Taproot& t);
  90 | +std::ostream& operator<<(std::ostream& os, const CTxDestination& dest);
  91 | +std::ostream& operator<<(std::ostream& os, const CPubKey& pk);
  92 | +std::ostream& operator<<(std::ostream& os, const KeyOriginInfo& info);
  93 | +std::ostream& operator<<(std::ostream& os, const ByRatioNegSize<FeeFrac>& b);
  94 | +

ismaelsadeeq commented at 7:31 PM on September 7, 2026:

In "test: Add display operator<< overloads to common.{h,cpp}" 05bd1513be2d9aaeb0a94671024f585f0d3cca15

Should we add for script too?

std::ostream& operator<<(std::ostream& os, const CScript& script);

rustaceanrob commented at 9:27 AM on September 8, 2026:

Yeah, definitely, I added a debug output and modified psbt_tests and script_tests to include it.

ismaelsadeeq commented at 7:34 PM on September 7, 2026: member

Code review ACK 056bd2582bc96f1fc2b47933badcd4fe85832c3c

Left a few comments. I checked the new test modification and did not see any change in the test invariants.

DrahtBot requested review from brunoerg on Sep 7, 2026

DrahtBot requested review from jeanpablojp on Sep 7, 2026

DrahtBot requested review from sedited on Sep 7, 2026

rustaceanrob force-pushed on Sep 8, 2026

DrahtBot added the label CI failed on Sep 8, 2026

DrahtBot removed the label CI failed on Sep 8, 2026

jeanpablojp commented at 1:13 PM on September 8, 2026: contributor

tACK 0560c013476708771cf9141fda54410cbee07645

Rebuilt and ran the unit tests on this head. Left one non-blocking nit.

test: Add display `operator<<` overloads to `common.{h,cpp}`

If the `<<` operator exists, boost can use this as a debug output when a
check fails (besides `BOOST_CHECK`). This adds `<<` for types commonly
used across the unit tests that do not yet have a display output.

ed9dd30570

test: Convert `BOOST_CHECK` macros with known debug string

Using the previous commit we can improve the error output of these
checks by updating the boost macros to a version that prints the values
on failure.

387e443275

in src/test/util/common.cpp:31 in 0560c01347 outdated

  26 | +std::ostream& operator<<(std::ostream& os, const AddressPosition& pos)
  27 | +{
  28 | +    return os << strprintf("AddressPosition(tried=%d, multiplicity=%d, bucket=%d, position=%d)", pos.tried, pos.multiplicity, pos.bucket, pos.position);
  29 | +}
  30 | +
  31 | +std::ostream& operator<<(std::ostream& os, const CScript& script)

jeanpablojp commented at 1:13 PM on September 8, 2026:

nit: ScriptToAsmStr decodes any push of up to four bytes as a number, so scripts that differ in bytes can come out as the same string.

A P2WPKH scriptPubKey and the same script with OP_0 replaced by a one-byte zero push, which is no longer a witness program, both print the same, and psbt_tests compares exactly GetScriptForDestination(WitnessV0KeyHash{...}). Would it be worth adding hex=%s alongside?

rustaceanrob commented at 1:31 PM on September 8, 2026:

Nice find. Somewhat unfortunate to make the debug output longer here, but given the ASM will be the most useful a majority of the time, with the possibility of needing to use hex, I added the hex string alongside the ASM in the latest push.

rustaceanrob force-pushed on Sep 8, 2026

ismaelsadeeq commented at 3:30 PM on September 8, 2026: member

reACK 387e443275587a46c25f21ed5e05e02439140a5a, all my suggestions were addressed.

DrahtBot requested review from jeanpablojp on Sep 8, 2026

jeanpablojp commented at 4:08 PM on September 8, 2026: contributor

re-ACK 387e443275587a46c25f21ed5e05e02439140a5a