test: Tighten Coin equality and add debug output #35965

rustaceanrob commented at 6:28 PM on August 13, 2026: member

If the == operator on two Coin fails, the developer should also see the conditions under which it failed. All that is required is adding a << operator, moving the == out of the namespace, and switching == sites to BOOST_TEST.

Here we also tighten what it means for a coin to be "equal."

This is a pre-requiste for #35713 but seems to be a benefit on its own.

DrahtBot added the label Refactoring on Aug 13, 2026

DrahtBot commented at 6:28 PM on August 13, 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/35965.

Reviews

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

Type	Reviewers
ACK	josibake, maflcko

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:

#30342 (kernel, logging: Pass Logger instances to kernel objects 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.

in src/test/coins_tests.cpp:44 in 4e5af1e554

  42 |             a.out == b.out;
  43 |  }
  44 |  
  45 | +static std::ostream& operator<<(std::ostream& out, const Coin& coin)
  46 | +{
  47 | +    return out << "Coin{spent=" << coin.IsSpent() << ", coinbase=" << coin.fCoinBase

l0rinc commented at 6:35 PM on August 13, 2026:

we could use an strprintf instead

in src/test/coins_tests.cpp:36 in 4e5af1e554

  34 | -{
  35 |  //! equality test
  36 | -bool operator==(const Coin &a, const Coin &b) {
  37 | +static bool operator==(const Coin &a, const Coin &b) {
  38 |      // Empty Coin objects are always equal.
  39 |      if (a.IsSpent() && b.IsSpent()) return true;

l0rinc commented at 6:36 PM on August 13, 2026:

I never liked this helper, all spent are considered equal - we should have a better way of testing this. What if we added proper equality check for coin and compared them using standard BOOST_CHECK_EQUAL instead, see https://github.com/l0rinc/bitcoin/pull/282/commits

l0rinc changes_requested

l0rinc commented at 12:41 AM on August 15, 2026: contributor

I agree that the test should be adjusted, but I don't think the current approach is going far enough. Please see my comments.

rustaceanrob force-pushed on Aug 17, 2026

rustaceanrob renamed this:
~~refactor: test: Add debug output for Coin in coins_tests~~
test: Tighten Coin equality and add debug output
on Aug 17, 2026

in src/test/coins_tests.cpp:162 in 4e1c45341e outdated

 158 | @@ -166,7 +159,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
 159 |              // former just delegates to the latter and returns the first unspent in a txn.
 160 |              const Coin& entry = (m_rng.randrange(500) == 0) ?
 161 |                  AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0));
 162 | -            BOOST_CHECK(coin == entry);
 163 | +            BOOST_TEST(coin == entry);

l0rinc commented at 6:23 PM on August 17, 2026:

we should be able to use BOOST_CHECK_EQUAL now

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

BOOST_TEST also includes the debug output but I can change this to CHECK_EQUAL if you feel strongly on which macro is used.

josibake commented at 12:02 PM on August 18, 2026:

we should be able to use BOOST_CHECK_EQUAL now

What's the benefit over BOOST_TEST?

l0rinc commented at 6:06 PM on August 18, 2026:

What's the benefit over BOOST_TEST?

One is used throughout the project:

% rg 'BOOST_CHECK_EQUAL\(' src | wc -l
    3673

while the other isn't:

% rg 'BOOST_TEST\([^;]+==' src | wc -l
      37

For simple values they both show the same error for the cases I tried...

<details><summary>Error output</summary>

> BOOST_TEST(cache.AccessCoin(outpoint) == coin2);

> BOOST_CHECK_EQUAL(cache.AccessCoin(outpoint), coin2);

test/coins_tests.cpp:1120: error: in "coins_tests/ccoins_addcoin_exception_keeps_usage_balanced": check cache.AccessCoin(outpoint) == coin2 has failed [Coin(spent=0, coinbase=0, height=1, value=4, scriptPubKey=25a0ed9994a2fea19823d9383e72c2ff13de62741338329401ebc9cbf661c2a280a6e61fe73f) != Coin(spent=0, coinbase=0, height=2, value=19, scriptPubKey=268bbfd02574fc17eacbd75786472ac43290da5cbd78836d75142252f36c717d725feff56ec773)]

</details>

... but I strongly dislike surprises and C++ and compiler magic (isn't that why we wanna get rid of boost in the first place?). While BOOST_CHECK_EQUAL(a, b) receives two separate macro arguments that can be compared and printed, BOOST_TEST(a == b) receives one expression, then uses C++ operator overloading and expression templates to capture its operands separately.

For example, migrating https://github.com/bitcoin/bitcoin/blob/e550945a3941e31c8a31983fdce29c11e584bea1/src/test/txgraph_tests.cpp#L300 to BOOST_TEST doesn't even compile without extra grouping.

Even worse, if we try to inject a bug in https://github.com/bitcoin/bitcoin/blob/e550945a3941e31c8a31983fdce29c11e584bea1/src/test/txgraph_tests.cpp#L88

        BOOST_CHECK_EQUAL(graph->Exists(refs[i], TxGraph::Level::TOP), i != NUM_BOTTOM_TX);

it fails with:

test/txgraph_tests.cpp:88: error: in "txgraph_tests/txgraph_trim_zigzag": check graph->Exists(refs[i], TxGraph::Level::TOP) == i != NUM_BOTTOM_TX has failed [true != false]

but the same would just pass with BOOST_TEST because the final != compares the preceding boolean result with NUM_BOTTOM_TX:

        BOOST_TEST(graph->Exists(refs[i], TxGraph::Level::TOP) == i != NUM_BOTTOM_TX);

BOOST_CHECK_EQUAL is verbose and ugly, but it's what we already use and BOOST_TEST has more surprises. If we want to migrate to BOOST_TEST anyway, let's not start it covertly in this PR, but have a dedicated migration PR with proper reasoning for why we would want that.

rustaceanrob commented at 8:49 AM on August 19, 2026:

Switched to BOOST_CHECK_EQUAL for the sake of this PR. "One is used throughout the project" seems like a fine reason for now. Agreed the migration can come later with separate motivation. Not sure it's worth getting into the internals of these two macros right now so I will mark this as resolved.

rustaceanrob force-pushed on Aug 18, 2026

josibake commented at 12:01 PM on August 18, 2026: member

ACK https://github.com/bitcoin/bitcoin/commit/28289ac0651052df08c3824836f0c5756d12ef17

Came here from diving into the state of #35713. Nice to see the tests DRY'd up.

test: Tighten `Coin` equality and add debug output

If the `==` operator on two `Coin` fails, the developer should also see
the conditions under which it failed. All that is required is adding a
`<<` operator, moving the `==` out of the namespace, and switching `==`
sites to `BOOST_TEST`.

Here we also tighten what it means for a coin to be "equal."

This is a pre-requiste for #35713 but seems to be a benefit on its own.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

1156ce6754

rustaceanrob force-pushed on Aug 19, 2026

josibake commented at 9:08 AM on August 19, 2026: member

reACK https://github.com/bitcoin/bitcoin/pull/35965/commits/1156ce675457064c95d653e40c7a611113ad864b

Small change moving BOOST_TEST -> BOOST_CHECK_EQUAL

maflcko commented at 10:32 AM on August 19, 2026: member

review ACK 1156ce675457064c95d653e40c7a611113ad864b 🔋

<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 1156ce675457064c95d653e40c7a611113ad864b 🔋
GOFOIe89vaFvpAm9Ky4kWgaRiAXaQhhfEUsBPJEgkuIpAQeHIxVto18fI2nE5JcUMylA0FGaWwPlsNEcvuLJDg==

</details>

fanquake merged this on Aug 19, 2026

fanquake closed this on Aug 19, 2026

l0rinc commented at 6:21 PM on August 19, 2026: contributor

post merge ACK 1156ce675457064c95d653e40c7a611113ad864b

rustaceanrob deleted the branch on Aug 20, 2026