This will also make the inducted test failure (mentioned in the commit message) nicer to read, and a lot faster.
I don’t expect the failure case to happen often, so the speed doesn’t really matter, but the error message is a lot clearer this way:
0 node.gettxout(txid, 0)
1 ~~~~~~~~~~~~~^^^^^^^^^
Thanks for the suggestion, updated in https://github.com/bitcoin/bitcoin/compare/4ae1f793d6718edf7a94fb032a2a7423105e85bd..36a01d082a860ae060c93525c9a0ffe974a9f729, added you as coauthor.
if the HaveCoin is dead code, then I suspect that #26331 did not fix #26112.
CCoinsViewDB::HaveCoin seems to be the reason for why HaveCoin was introduced in the first place https://github.com/bitcoin/bitcoin/blob/d3063740684f51722850e6ffec290951913fa56d/src/txdb.cpp#L81-L90
in other cases we could just call GetCoin (most implementations actually do).
The underlying Exists https://github.com/bitcoin/bitcoin/blob/a9b7f5614c24fe6f386448604c325ec4fa6c98a5/src/dbwrapper.h#L234-L241 avoid serialization of Read https://github.com/bitcoin/bitcoin/blob/a9b7f5614c24fe6f386448604c325ec4fa6c98a5/src/dbwrapper.h#L206-L224
Explicitly throwing from CCoinsViewDB::HaveCoin and replacing the test-only HaveCoin calls with GetCoin in coins_tests.cpp indicates that we don’t have any production HaveCoin calls going to disk. The same is confirmed by keeping the exception and starting bitcoind.
0diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
1index 6396fce60a..c6c8fb9229 100644
2--- a/src/test/coins_tests.cpp
3+++ b/src/test/coins_tests.cpp
4@@ -160,7 +160,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
5 bool test_havecoin_before = m_rng.randbits(2) == 0;
6 bool test_havecoin_after = m_rng.randbits(2) == 0;
7
8- bool result_havecoin = test_havecoin_before ? stack.back()->HaveCoin(COutPoint(txid, 0)) : false;
9+ bool result_havecoin = test_havecoin_before ? !!stack.back()->GetCoin(COutPoint(txid, 0)) : false;
10
11 // Infrequently, test usage of AccessByTxid instead of AccessCoin - the
12 // former just delegates to the latter and returns the first unspent in a txn.
13@@ -173,7 +173,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
14 }
15
16 if (test_havecoin_after) {
17- bool ret = stack.back()->HaveCoin(COutPoint(txid, 0));
18+ bool ret = !!stack.back()->GetCoin(COutPoint(txid, 0));
19 BOOST_CHECK(ret == !entry.IsSpent());
20 }
21
22@@ -214,7 +214,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
23 // Once every 1000 iterations and at the end, verify the full cache.
24 if (m_rng.randrange(1000) == 1 || i == NUM_SIMULATION_ITERATIONS - 1) {
25 for (const auto& entry : result) {
26- bool have = stack.back()->HaveCoin(entry.first);
27+ bool have = !!stack.back()->GetCoin(entry.first);
28 const Coin& coin = stack.back()->AccessCoin(entry.first);
29 BOOST_CHECK(have == !coin.IsSpent());
30 BOOST_CHECK(coin == entry.second);
31@@ -473,7 +473,7 @@ BOOST_FIXTURE_TEST_CASE(updatecoins_simulation_test, UpdateTest)
32 // Once every 1000 iterations and at the end, verify the full cache.
33 if (m_rng.randrange(1000) == 1 || i == NUM_SIMULATION_ITERATIONS - 1) {
34 for (const auto& entry : result) {
35- bool have = stack.back()->HaveCoin(entry.first);
36+ bool have = !!stack.back()->GetCoin(entry.first);
37 const Coin& coin = stack.back()->AccessCoin(entry.first);
38 BOOST_CHECK(have == !coin.IsSpent());
39 BOOST_CHECK(coin == entry.second);
40@@ -920,8 +920,8 @@ void TestFlushBehavior(
41 COutPoint outp = COutPoint(txid, 0);
42 Coin coin = MakeCoin();
43 // Ensure the coins views haven't seen this coin before.
44- BOOST_CHECK(!base.HaveCoin(outp));
45- BOOST_CHECK(!view->HaveCoin(outp));
46+ BOOST_CHECK(!base.GetCoin(outp));
47+ BOOST_CHECK(!view->GetCoin(outp));
48
49 // --- 1. Adding a random coin to the child cache
50 //
51@@ -931,8 +931,8 @@ void TestFlushBehavior(
52 cache_size = view->map().size();
53
54 // `base` shouldn't have coin (no flush yet) but `view` should have cached it.
55- BOOST_CHECK(!base.HaveCoin(outp));
56- BOOST_CHECK(view->HaveCoin(outp));
57+ BOOST_CHECK(!base.GetCoin(outp));
58+ BOOST_CHECK(!!view->GetCoin(outp));
59
60 BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::DIRTY_FRESH));
61
62@@ -949,8 +949,8 @@ void TestFlushBehavior(
63 BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::CLEAN)); // State should have been wiped.
64
65 // Both views should now have the coin.
66- BOOST_CHECK(base.HaveCoin(outp));
67- BOOST_CHECK(view->HaveCoin(outp));
68+ BOOST_CHECK(!!base.GetCoin(outp));
69+ BOOST_CHECK(!!view->GetCoin(outp));
70
71 if (do_erasing_flush) {
72 // --- 4. Flushing the caches again (with erasing)
73@@ -981,14 +981,14 @@ void TestFlushBehavior(
74
75 // The coin should be in the cache, but spent and marked dirty.
76 BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), SPENT_DIRTY);
77- BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`.
78- BOOST_CHECK(base.HaveCoin(outp)); // But coin should still be unspent in `base`.
79+ BOOST_CHECK(!view->GetCoin(outp)); // Coin should be considered spent in `view`.
80+ BOOST_CHECK(!!base.GetCoin(outp)); // But coin should still be unspent in `base`.
81
82 flush_all(/*erase=*/ false);
83
84 // Coin should be considered spent in both views.
85- BOOST_CHECK(!view->HaveCoin(outp));
86- BOOST_CHECK(!base.HaveCoin(outp));
87+ BOOST_CHECK(!view->GetCoin(outp));
88+ BOOST_CHECK(!base.GetCoin(outp));
89
90 // Spent coin should not be spendable.
91 BOOST_CHECK(!view->SpendCoin(outp));
92@@ -999,21 +999,21 @@ void TestFlushBehavior(
93 txid = Txid::FromUint256(m_rng.rand256());
94 outp = COutPoint(txid, 0);
95 coin = MakeCoin();
96- BOOST_CHECK(!base.HaveCoin(outp));
97- BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
98- BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
99+ BOOST_CHECK(!base.GetCoin(outp));
100+ BOOST_CHECK(!all_caches[0]->GetCoin(outp));
101+ BOOST_CHECK(!all_caches[1]->GetCoin(outp));
102
103 all_caches[0]->AddCoin(outp, std::move(coin), false);
104 all_caches[0]->Sync();
105- BOOST_CHECK(base.HaveCoin(outp));
106- BOOST_CHECK(all_caches[0]->HaveCoin(outp));
107+ BOOST_CHECK(!!base.GetCoin(outp));
108+ BOOST_CHECK(!!all_caches[0]->GetCoin(outp));
109 BOOST_CHECK(!all_caches[1]->HaveCoinInCache(outp));
110
111 BOOST_CHECK(all_caches[1]->SpendCoin(outp));
112 flush_all(/*erase=*/ false);
113- BOOST_CHECK(!base.HaveCoin(outp));
114- BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
115- BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
116+ BOOST_CHECK(!base.GetCoin(outp));
117+ BOOST_CHECK(!all_caches[0]->GetCoin(outp));
118+ BOOST_CHECK(!all_caches[1]->GetCoin(outp));
119
120 flush_all(/*erase=*/ true); // Erase all cache content.
121
122@@ -1023,9 +1023,9 @@ void TestFlushBehavior(
123 outp = COutPoint(txid, 0);
124 coin = MakeCoin();
125 CAmount coin_val = coin.out.nValue;
126- BOOST_CHECK(!base.HaveCoin(outp));
127- BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
128- BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
129+ BOOST_CHECK(!base.GetCoin(outp));
130+ BOOST_CHECK(!all_caches[0]->GetCoin(outp));
131+ BOOST_CHECK(!all_caches[1]->GetCoin(outp));
132
133 // Add and spend from same cache without flushing.
134 all_caches[0]->AddCoin(outp, std::move(coin), false);
135@@ -1033,7 +1033,7 @@ void TestFlushBehavior(
136 // Coin should be FRESH in the cache.
137 BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(coin_val, CoinEntry::State::DIRTY_FRESH));
138 // Base shouldn't have seen coin.
139- BOOST_CHECK(!base.HaveCoin(outp));
140+ BOOST_CHECK(!base.GetCoin(outp));
141
142 BOOST_CHECK(all_caches[0]->SpendCoin(outp));
143 all_caches[0]->Sync();
144@@ -1041,7 +1041,7 @@ void TestFlushBehavior(
145 // Ensure there is no sign of the coin after spend/flush.
146 BOOST_CHECK(!GetCoinsMapEntry(all_caches[0]->map(), outp));
147 BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp));
148- BOOST_CHECK(!base.HaveCoin(outp));
149+ BOOST_CHECK(!base.GetCoin(outp));
150 }
151 }; // struct FlushTest
152
153diff --git a/src/txdb.cpp b/src/txdb.cpp
154index 1dc20717bc..6613b72f7b 100644
155--- a/src/txdb.cpp
156+++ b/src/txdb.cpp
157@@ -80,13 +80,7 @@ std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint) const
158
159 bool CCoinsViewDB::HaveCoin(const COutPoint& outpoint) const
160 {
161- try {
162- return m_db->Exists(CoinEntry(&outpoint));
163- } catch (const std::runtime_error& e) {
164- m_read_error_cb();
165- LogError("Database error in HaveCoin: %s", e.what());
166- std::abort();
167- }
168+ throw "CCoinsViewDB::HaveCoin"; // TODO see who fails this way
169 }
170
171 uint256 CCoinsViewDB::GetBestBlock() const {
#26331 did not fix #26112.
I will investigate that, thanks for bringing them to my attention.
Edit: i686, no IPC disagreed with the previous exceptions, trying something broader: https://github.com/bitcoin/bitcoin/actions/runs/20928505545/job/60133010356?pr=34132
