test: Add debug output to common tested types #36091

pull rustaceanrob wants to merge 2 commits into bitcoin:master from rustaceanrob:26-8-25-display changing 14 files +285 −108

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

Concept ACK sedited, brunoerg, jeanpablojp

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)
- #36087 (util: Add and use AssertUnreachable by maflcko)
- #36074 (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)
- #35713 (Remove boost as a unit test runner by rustaceanrob)
- #35511 (RFC: consensus: Make CAmount a class by hodlinator)
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.
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

Type	Reviewers
Concept ACK	sedited, brunoerg, jeanpablojp

test: Add display header for tested types

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.

dacc99b190

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.

f9273c8c6d

rustaceanrob force-pushed on Aug 31, 2026

Contributors

Labels

Linked (view graph)

#35713 Remove boost as a unit test runner

test: Add debug output to common tested types #36091

Code Coverage & Benchmarks

Reviews

Conflicts