Merge bitcoin/bitcoin#35965: test: Tighten Coin equality and add debug output

1156ce6754 test: Tighten `Coin` equality and add debug output (rustaceanrob)

Pull request description:

  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 https://github.com/bitcoin/bitcoin/pull/35713 but seems to be a benefit on its own.

ACKs for top commit:
  josibake:
    reACK 1156ce6754
  maflcko:
    review ACK 1156ce6754 🔋

Tree-SHA512: de5c612998518371ded3d25abdf1c96640e33d9961902dc4765a7e8f5088d8698a66ad63bc0a9822ec2b53e41e72dc95f78196822429af30b2ec29baa31c1ed1

src/test/coins_tests.cpp

@@ -6,6 +6,7 @@
 #include <clientversion.h>
 #include <coins.h>
 #include <streams.h>
+#include <test/util/coins.h>
 #include <test/util/common.h>
 #include <test/util/poolresourcetester.h>
 #include <test/util/random.h>
@@ -31,14 +32,6 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund
 
 namespace
 {
-//! equality test
-bool operator==(const Coin &a, const Coin &b) {
-    // Empty Coin objects are always equal.
-    if (a.IsSpent() && b.IsSpent()) return true;
-    return a.fCoinBase == b.fCoinBase &&
-           a.nHeight == b.nHeight &&
-           a.out == b.out;
-}
 
 class CCoinsViewTest : public CoinsViewEmpty
 {
@@ -166,7 +159,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
             // former just delegates to the latter and returns the first unspent in a txn.
             const Coin& entry = (m_rng.randrange(500) == 0) ?
                 AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0));
-            BOOST_CHECK(coin == entry);
+            BOOST_CHECK_EQUAL(coin, entry);
 
             if (test_havecoin_before) {
                 BOOST_CHECK(result_havecoin == !entry.IsSpent());
@@ -220,7 +213,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
                 bool have = stack.back()->HaveCoin(entry.first);
                 const Coin& coin = stack.back()->AccessCoin(entry.first);
                 BOOST_CHECK(have == !coin.IsSpent());
-                BOOST_CHECK(coin == entry.second);
+                BOOST_CHECK_EQUAL(coin, entry.second);
                 if (coin.IsSpent()) {
                     missed_an_entry = true;
                 } else {
@@ -479,7 +472,7 @@ BOOST_FIXTURE_TEST_CASE(updatecoins_simulation_test, UpdateTest)
                 bool have = stack.back()->HaveCoin(entry.first);
                 const Coin& coin = stack.back()->AccessCoin(entry.first);
                 BOOST_CHECK(have == !coin.IsSpent());
-                BOOST_CHECK(coin == entry.second);
+                BOOST_CHECK_EQUAL(coin, entry.second);
             }
         }
 
@@ -1081,7 +1074,7 @@ BOOST_FIXTURE_TEST_CASE(coins_db_leveldb_layout, FlushTest)
     WITH_LOCK(::cs_main, return base.CompactFullAsync()).wait();
     BOOST_CHECK_EQUAL(level2_files(base), 1);
 
-    BOOST_CHECK(*Assert(base.GetCoin(outpoint)) == coin);
+    BOOST_CHECK_EQUAL(*Assert(base.GetCoin(outpoint)), coin);
     BOOST_CHECK_EQUAL(base.GetBestBlock(), block_hash);
 }
 
@@ -1124,7 +1117,7 @@ BOOST_AUTO_TEST_CASE(ccoins_addcoin_exception_keeps_usage_balanced)
     BOOST_CHECK_THROW(cache.AddCoin(outpoint, Coin{coin2}, /*possible_overwrite=*/false), std::logic_error);
     cache.SelfTest();
 
-    BOOST_CHECK(cache.AccessCoin(outpoint) == coin1);
+    BOOST_CHECK_EQUAL(cache.AccessCoin(outpoint), coin1);
 }
 
 BOOST_AUTO_TEST_CASE(ccoins_emplace_duplicate_keeps_usage_balanced)
@@ -1141,7 +1134,7 @@ BOOST_AUTO_TEST_CASE(ccoins_emplace_duplicate_keeps_usage_balanced)
     cache.EmplaceCoinInternalDANGER(outpoint, Coin{coin2});
     cache.SelfTest();
 
-    BOOST_CHECK(cache.AccessCoin(outpoint) == coin1);
+    BOOST_CHECK_EQUAL(cache.AccessCoin(outpoint), coin1);
 }
 
 BOOST_AUTO_TEST_CASE(ccoins_reset_guard)
@@ -1165,7 +1158,7 @@ BOOST_AUTO_TEST_CASE(ccoins_reset_guard)
 
     {
         const auto reset_guard{cache.CreateResetGuard()};
-        BOOST_CHECK(cache.AccessCoin(outpoint) == coin);
+        BOOST_CHECK_EQUAL(cache.AccessCoin(outpoint), coin);
         BOOST_CHECK(!cache.AccessCoin(outpoint).IsSpent());
         BOOST_CHECK_EQUAL(cache.GetCacheSize(), 1);
         BOOST_CHECK_EQUAL(cache.GetDirtyCount(), 1);
@@ -1212,7 +1205,7 @@ BOOST_AUTO_TEST_CASE(ccoins_peekcoin)
     CCoinsViewCacheTest main_cache{&base};
     const auto fetched{main_cache.PeekCoin(outpoint)};
     BOOST_CHECK(fetched.has_value());
-    BOOST_CHECK(*fetched == coin);
+    BOOST_CHECK_EQUAL(*fetched, coin);
     BOOST_CHECK(!main_cache.HaveCoinInCache(outpoint));
 }
 

src/test/fuzz/coins_view.cpp

@@ -16,6 +16,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <test/util/coins.h>
 #include <test/util/setup_common.h>
 #include <txdb.h>
 #include <util/hasher.h>
@@ -35,14 +36,6 @@
 #include <vector>
 
 namespace {
-const Coin EMPTY_COIN{};
-
-bool operator==(const Coin& a, const Coin& b)
-{
-    if (a.IsSpent() && b.IsSpent()) return true;
-    return a.fCoinBase == b.fCoinBase && a.nHeight == b.nHeight && a.out == b.out;
-}
-
 /**
  * MutationGuardCoinsViewCache asserts that nothing mutates cacheCoins until
  * BatchWrite is called. It keeps a snapshot of the cacheCoins state, which it
@@ -370,7 +363,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co
 
     {
         const Coin& coin_using_access_coin = coins_view_cache.AccessCoin(random_out_point);
-        const bool exists_using_access_coin = !(coin_using_access_coin == EMPTY_COIN);
+        const bool exists_using_access_coin = !coin_using_access_coin.IsSpent();
         const bool exists_using_have_coin = coins_view_cache.HaveCoin(random_out_point);
         const bool exists_using_have_coin_in_cache = coins_view_cache.HaveCoinInCache(random_out_point);
         if (auto coin{coins_view_cache.GetCoin(random_out_point)}) {

src/test/util/coins.h

@@ -5,7 +5,12 @@
 #ifndef BITCOIN_TEST_UTIL_COINS_H
 #define BITCOIN_TEST_UTIL_COINS_H
 
+#include <coins.h>
+#include <crypto/hex_base.h>
 #include <primitives/transaction.h>
+#include <tinyformat.h>
+
+#include <ostream>
 
 class CCoinsViewCache;
 class FastRandomContext;
@@ -17,4 +22,17 @@ class FastRandomContext;
  */
 COutPoint AddTestCoin(FastRandomContext& rng, CCoinsViewCache& coins_view);
 
+//! Strict equality, including fields of spent coins
+inline bool operator==(const Coin& a, const Coin& b)
+{
+    return a.fCoinBase == b.fCoinBase && a.nHeight == b.nHeight && a.out == b.out;
+}
+
+//! Printed when a coin comparison fails
+inline std::ostream& operator<<(std::ostream& os, const Coin& coin)
+{
+    return os << strprintf("Coin(spent=%d, coinbase=%d, height=%d, value=%d, scriptPubKey=%s)",
+                           coin.IsSpent(), coin.fCoinBase, coin.nHeight, coin.out.nValue, HexStr(coin.out.scriptPubKey));
+}
+
 #endif // BITCOIN_TEST_UTIL_COINS_H