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:
node.gettxout(txid, 0)
~~~~~~~~~~~~~^^^^^^^^^
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.
<details>
<summary>Patch</summary>
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 6396fce60a..c6c8fb9229 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -160,7 +160,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
bool test_havecoin_before = m_rng.randbits(2) == 0;
bool test_havecoin_after = m_rng.randbits(2) == 0;
- bool result_havecoin = test_havecoin_before ? stack.back()->HaveCoin(COutPoint(txid, 0)) : false;
+ bool result_havecoin = test_havecoin_before ? !!stack.back()->GetCoin(COutPoint(txid, 0)) : false;
// Infrequently, test usage of AccessByTxid instead of AccessCoin - the
// former just delegates to the latter and returns the first unspent in a txn.
@@ -173,7 +173,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
}
if (test_havecoin_after) {
- bool ret = stack.back()->HaveCoin(COutPoint(txid, 0));
+ bool ret = !!stack.back()->GetCoin(COutPoint(txid, 0));
BOOST_CHECK(ret == !entry.IsSpent());
}
@@ -214,7 +214,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block)
// Once every 1000 iterations and at the end, verify the full cache.
if (m_rng.randrange(1000) == 1 || i == NUM_SIMULATION_ITERATIONS - 1) {
for (const auto& entry : result) {
- bool have = stack.back()->HaveCoin(entry.first);
+ bool have = !!stack.back()->GetCoin(entry.first);
const Coin& coin = stack.back()->AccessCoin(entry.first);
BOOST_CHECK(have == !coin.IsSpent());
BOOST_CHECK(coin == entry.second);
@@ -473,7 +473,7 @@ BOOST_FIXTURE_TEST_CASE(updatecoins_simulation_test, UpdateTest)
// Once every 1000 iterations and at the end, verify the full cache.
if (m_rng.randrange(1000) == 1 || i == NUM_SIMULATION_ITERATIONS - 1) {
for (const auto& entry : result) {
- bool have = stack.back()->HaveCoin(entry.first);
+ bool have = !!stack.back()->GetCoin(entry.first);
const Coin& coin = stack.back()->AccessCoin(entry.first);
BOOST_CHECK(have == !coin.IsSpent());
BOOST_CHECK(coin == entry.second);
@@ -920,8 +920,8 @@ void TestFlushBehavior(
COutPoint outp = COutPoint(txid, 0);
Coin coin = MakeCoin();
// Ensure the coins views haven't seen this coin before.
- BOOST_CHECK(!base.HaveCoin(outp));
- BOOST_CHECK(!view->HaveCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
+ BOOST_CHECK(!view->GetCoin(outp));
// --- 1. Adding a random coin to the child cache
//
@@ -931,8 +931,8 @@ void TestFlushBehavior(
cache_size = view->map().size();
// `base` shouldn't have coin (no flush yet) but `view` should have cached it.
- BOOST_CHECK(!base.HaveCoin(outp));
- BOOST_CHECK(view->HaveCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
+ BOOST_CHECK(!!view->GetCoin(outp));
BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::DIRTY_FRESH));
@@ -949,8 +949,8 @@ void TestFlushBehavior(
BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), CoinEntry(coin.out.nValue, CoinEntry::State::CLEAN)); // State should have been wiped.
// Both views should now have the coin.
- BOOST_CHECK(base.HaveCoin(outp));
- BOOST_CHECK(view->HaveCoin(outp));
+ BOOST_CHECK(!!base.GetCoin(outp));
+ BOOST_CHECK(!!view->GetCoin(outp));
if (do_erasing_flush) {
// --- 4. Flushing the caches again (with erasing)
@@ -981,14 +981,14 @@ void TestFlushBehavior(
// The coin should be in the cache, but spent and marked dirty.
BOOST_CHECK_EQUAL(GetCoinsMapEntry(view->map(), outp), SPENT_DIRTY);
- BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`.
- BOOST_CHECK(base.HaveCoin(outp)); // But coin should still be unspent in `base`.
+ BOOST_CHECK(!view->GetCoin(outp)); // Coin should be considered spent in `view`.
+ BOOST_CHECK(!!base.GetCoin(outp)); // But coin should still be unspent in `base`.
flush_all(/*erase=*/ false);
// Coin should be considered spent in both views.
- BOOST_CHECK(!view->HaveCoin(outp));
- BOOST_CHECK(!base.HaveCoin(outp));
+ BOOST_CHECK(!view->GetCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
// Spent coin should not be spendable.
BOOST_CHECK(!view->SpendCoin(outp));
@@ -999,21 +999,21 @@ void TestFlushBehavior(
txid = Txid::FromUint256(m_rng.rand256());
outp = COutPoint(txid, 0);
coin = MakeCoin();
- BOOST_CHECK(!base.HaveCoin(outp));
- BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
- BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
+ BOOST_CHECK(!all_caches[0]->GetCoin(outp));
+ BOOST_CHECK(!all_caches[1]->GetCoin(outp));
all_caches[0]->AddCoin(outp, std::move(coin), false);
all_caches[0]->Sync();
- BOOST_CHECK(base.HaveCoin(outp));
- BOOST_CHECK(all_caches[0]->HaveCoin(outp));
+ BOOST_CHECK(!!base.GetCoin(outp));
+ BOOST_CHECK(!!all_caches[0]->GetCoin(outp));
BOOST_CHECK(!all_caches[1]->HaveCoinInCache(outp));
BOOST_CHECK(all_caches[1]->SpendCoin(outp));
flush_all(/*erase=*/ false);
- BOOST_CHECK(!base.HaveCoin(outp));
- BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
- BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
+ BOOST_CHECK(!all_caches[0]->GetCoin(outp));
+ BOOST_CHECK(!all_caches[1]->GetCoin(outp));
flush_all(/*erase=*/ true); // Erase all cache content.
@@ -1023,9 +1023,9 @@ void TestFlushBehavior(
outp = COutPoint(txid, 0);
coin = MakeCoin();
CAmount coin_val = coin.out.nValue;
- BOOST_CHECK(!base.HaveCoin(outp));
- BOOST_CHECK(!all_caches[0]->HaveCoin(outp));
- BOOST_CHECK(!all_caches[1]->HaveCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
+ BOOST_CHECK(!all_caches[0]->GetCoin(outp));
+ BOOST_CHECK(!all_caches[1]->GetCoin(outp));
// Add and spend from same cache without flushing.
all_caches[0]->AddCoin(outp, std::move(coin), false);
@@ -1033,7 +1033,7 @@ void TestFlushBehavior(
// Coin should be FRESH in the cache.
BOOST_CHECK_EQUAL(GetCoinsMapEntry(all_caches[0]->map(), outp), CoinEntry(coin_val, CoinEntry::State::DIRTY_FRESH));
// Base shouldn't have seen coin.
- BOOST_CHECK(!base.HaveCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
BOOST_CHECK(all_caches[0]->SpendCoin(outp));
all_caches[0]->Sync();
@@ -1041,7 +1041,7 @@ void TestFlushBehavior(
// Ensure there is no sign of the coin after spend/flush.
BOOST_CHECK(!GetCoinsMapEntry(all_caches[0]->map(), outp));
BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp));
- BOOST_CHECK(!base.HaveCoin(outp));
+ BOOST_CHECK(!base.GetCoin(outp));
}
}; // struct FlushTest
diff --git a/src/txdb.cpp b/src/txdb.cpp
index 1dc20717bc..6613b72f7b 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -80,13 +80,7 @@ std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint) const
bool CCoinsViewDB::HaveCoin(const COutPoint& outpoint) const
{
- try {
- return m_db->Exists(CoinEntry(&outpoint));
- } catch (const std::runtime_error& e) {
- m_read_error_cb();
- LogError("Database error in HaveCoin: %s", e.what());
- std::abort();
- }
+ throw "CCoinsViewDB::HaveCoin"; // TODO see who fails this way
}
uint256 CCoinsViewDB::GetBestBlock() const {
</details>
#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
