From 6da6f503a6dd8de85780ca402f5202095aa8b6ea Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Sun, 12 Oct 2025 23:21:01 +0200 Subject: [PATCH] refactor: Let CCoinsViewCache::BatchWrite return void CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush. This allows for dropping a FatalError condition and simplifying some dead error handling code a bit. Since we now no longer exercise the "error path" when returning from `CCoinsView::BatchWrite`, make the method clear the cache instead. This should only be exercised by tests and not change production behaviour. This might slightly improve the coins_view fuzz test's ability to generate better coverage. Co-authored-by: l0rinc --- src/coins.cpp | 41 ++++++++++++++--------------- src/coins.h | 12 ++++----- src/test/coins_tests.cpp | 13 +++++---- src/test/fuzz/coins_view.cpp | 4 +-- src/test/fuzz/coinscache_sim.cpp | 3 +-- src/test/validation_flush_tests.cpp | 2 +- src/txdb.cpp | 4 +-- src/txdb.h | 2 +- src/validation.cpp | 10 +++---- 9 files changed, 41 insertions(+), 50 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 1ca5fca3e69..1a0a8f807c1 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -16,7 +16,11 @@ TRACEPOINT_SEMAPHORE(utxocache, uncache); std::optional CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; } uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector CCoinsView::GetHeadBlocks() const { return std::vector(); } -bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; } +void CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) +{ + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { } +} + std::unique_ptr CCoinsView::Cursor() const { return nullptr; } bool CCoinsView::HaveCoin(const COutPoint &outpoint) const @@ -30,7 +34,7 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base-> uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); } void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; } -bool CCoinsViewBacked::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return base->BatchWrite(cursor, hashBlock); } +void CCoinsViewBacked::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) { base->BatchWrite(cursor, hashBlock); } std::unique_ptr CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } @@ -183,7 +187,8 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { hashBlock = hashBlockIn; } -bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlockIn) { +void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlockIn) +{ for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { if (!it->second.IsDirty()) { // TODO a cursor can only contain dirty entries continue; @@ -246,33 +251,27 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha } } hashBlock = hashBlockIn; - return true; } -bool CCoinsViewCache::Flush(bool will_reuse_cache) { +void CCoinsViewCache::Flush(bool will_reuse_cache) +{ auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/true)}; - bool fOk = base->BatchWrite(cursor, hashBlock); - if (fOk) { - cacheCoins.clear(); - if (will_reuse_cache) { - ReallocateCache(); - } - cachedCoinsUsage = 0; + base->BatchWrite(cursor, hashBlock); + cacheCoins.clear(); + if (will_reuse_cache) { + ReallocateCache(); } - return fOk; + cachedCoinsUsage = 0; } -bool CCoinsViewCache::Sync() +void CCoinsViewCache::Sync() { auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/false)}; - bool fOk = base->BatchWrite(cursor, hashBlock); - if (fOk) { - if (m_sentinel.second.Next() != &m_sentinel) { - /* BatchWrite must clear flags of all entries */ - throw std::logic_error("Not all unspent flagged entries were cleared"); - } + base->BatchWrite(cursor, hashBlock); + if (m_sentinel.second.Next() != &m_sentinel) { + /* BatchWrite must clear flags of all entries */ + throw std::logic_error("Not all unspent flagged entries were cleared"); } - return fOk; } void CCoinsViewCache::Uncache(const COutPoint& hash) diff --git a/src/coins.h b/src/coins.h index 8d07f7fe9fd..6da53829996 100644 --- a/src/coins.h +++ b/src/coins.h @@ -324,7 +324,7 @@ public: //! Do a bulk modification (multiple Coin changes + BestBlock change). //! The passed cursor is used to iterate through the coins. - virtual bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock); + virtual void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock); //! Get a cursor to iterate over the whole state virtual std::unique_ptr Cursor() const; @@ -350,7 +350,7 @@ public: uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; void SetBackend(CCoinsView &viewIn); - bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; + void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override; std::unique_ptr Cursor() const override; size_t EstimateSize() const override; }; @@ -389,7 +389,7 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); - bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; + void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override; std::unique_ptr Cursor() const override { throw std::logic_error("CCoinsViewCache cursor iteration not supported."); } @@ -441,18 +441,16 @@ public: * to be forgotten. * If will_reuse_cache is false, the cache will retain the same memory footprint * after flushing and should be destroyed to deallocate. - * If false is returned, the state of this cache (and its backing view) will be undefined. */ - bool Flush(bool will_reuse_cache = true); + void Flush(bool will_reuse_cache = true); /** * Push the modifications applied to this cache to its base while retaining * the contents of this cache (except for spent coins, which we erase). * Failure to call this method or Flush() before destruction will cause the changes * to be forgotten. - * If false is returned, the state of this cache (and its backing view) will be undefined. */ - bool Sync(); + void Sync(); /** * Removes the UTXO with the given outpoint from the cache, if it is diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index a1152d245f7..e07164daf88 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -58,7 +58,7 @@ public: uint256 GetBestBlock() const override { return hashBestBlock_; } - bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override + void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override { for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)){ if (it->second.IsDirty()) { @@ -72,7 +72,6 @@ public: } if (!hashBlock.IsNull()) hashBestBlock_ = hashBlock; - return true; } }; @@ -237,7 +236,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) unsigned int flushIndex = m_rng.randrange(stack.size() - 1); if (fake_best_block) stack[flushIndex]->SetBestBlock(m_rng.rand256()); bool should_erase = m_rng.randrange(4) < 3; - BOOST_CHECK(should_erase ? stack[flushIndex]->Flush() : stack[flushIndex]->Sync()); + should_erase ? stack[flushIndex]->Flush() : stack[flushIndex]->Sync(); flushed_without_erase |= !should_erase; } } @@ -247,7 +246,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) //Remove the top cache if (fake_best_block) stack.back()->SetBestBlock(m_rng.rand256()); bool should_erase = m_rng.randrange(4) < 3; - BOOST_CHECK(should_erase ? stack.back()->Flush() : stack.back()->Sync()); + should_erase ? stack.back()->Flush() : stack.back()->Sync(); flushed_without_erase |= !should_erase; stack.pop_back(); } @@ -496,13 +495,13 @@ BOOST_FIXTURE_TEST_CASE(updatecoins_simulation_test, UpdateTest) // Every 100 iterations, flush an intermediate cache if (stack.size() > 1 && m_rng.randbool() == 0) { unsigned int flushIndex = m_rng.randrange(stack.size() - 1); - BOOST_CHECK(stack[flushIndex]->Flush()); + stack[flushIndex]->Flush(); } } if (m_rng.randrange(100) == 0) { // Every 100 iterations, change the cache stack. if (stack.size() > 0 && m_rng.randbool() == 0) { - BOOST_CHECK(stack.back()->Flush()); + stack.back()->Flush(); stack.pop_back(); } if (stack.size() == 0 || (stack.size() < 4 && m_rng.randbool())) { @@ -664,7 +663,7 @@ static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; if (cache_coin) InsertCoinsMapEntry(map, sentinel, *cache_coin); auto cursor{CoinsViewCacheCursor(sentinel, map, /*will_erase=*/true)}; - BOOST_CHECK(view.BatchWrite(cursor, {})); + view.BatchWrite(cursor, {}); } class SingleEntryCacheTest diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 9990e9cb4bc..09595678ad9 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -74,10 +74,10 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend } }, [&] { - (void)coins_view_cache.Flush(/*will_reuse_cache=*/fuzzed_data_provider.ConsumeBool()); + coins_view_cache.Flush(/*will_reuse_cache=*/fuzzed_data_provider.ConsumeBool()); }, [&] { - (void)coins_view_cache.Sync(); + coins_view_cache.Sync(); }, [&] { uint256 best_block{ConsumeUInt256(fuzzed_data_provider)}; diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 82e5f1fc437..e2ca09a7482 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -163,7 +163,7 @@ public: std::unique_ptr Cursor() const final { return {}; } size_t EstimateSize() const final { return m_data.size(); } - bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final + void BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final { for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { if (it->second.IsDirty()) { @@ -187,7 +187,6 @@ public: } } } - return true; } }; diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp index 2477cb19d60..66c284b9791 100644 --- a/src/test/validation_flush_tests.cpp +++ b/src/test/validation_flush_tests.cpp @@ -60,7 +60,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate) // CRITICAL → OK via Flush BOOST_CHECK_EQUAL(chainstate.GetCoinsCacheSizeState(MAX_COINS_BYTES, /*max_mempool_size_bytes=*/0), CoinsCacheSizeState::CRITICAL); view.SetBestBlock(m_rng.rand256()); - BOOST_REQUIRE(view.Flush()); + view.Flush(); BOOST_CHECK_EQUAL(chainstate.GetCoinsCacheSizeState(MAX_COINS_BYTES, /*max_mempool_size_bytes=*/0), CoinsCacheSizeState::OK); } diff --git a/src/txdb.cpp b/src/txdb.cpp index 843cb9b1be2..abe76c44c95 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -90,7 +90,8 @@ std::vector CCoinsViewDB::GetHeadBlocks() const { return vhashHeadBlocks; } -bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { +void CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) +{ CDBBatch batch(*m_db); size_t count = 0; size_t changed = 0; @@ -151,7 +152,6 @@ bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashB LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0)); m_db->WriteBatch(batch); LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count); - return true; } size_t CCoinsViewDB::EstimateSize() const diff --git a/src/txdb.h b/src/txdb.h index ea0cf9d77e5..272db9cac27 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -44,7 +44,7 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; - bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; + void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override; std::unique_ptr Cursor() const override; //! Whether an unsupported database format is used. diff --git a/src/validation.cpp b/src/validation.cpp index 3464e588456..87bc6410ef4 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2827,9 +2827,7 @@ bool Chainstate::FlushStateToDisk( } // Flush the chainstate (which may refer to block index entries). const auto empty_cache{(mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical}; - if (empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()) { - return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to coin database.")); - } + empty_cache ? CoinsTip().Flush() : CoinsTip().Sync(); full_flush_completed = true; TRACEPOINT(utxocache, flush, int64_t{Ticks(NodeClock::now() - nNow)}, @@ -2966,8 +2964,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra LogError("DisconnectTip(): DisconnectBlock %s failed\n", pindexDelete->GetBlockHash().ToString()); return false; } - bool flushed = view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope - assert(flushed); + view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope } LogDebug(BCLog::BENCH, "- Disconnect block: %.2fms\n", Ticks(SteadyClock::now() - time_start)); @@ -3101,8 +3098,7 @@ bool Chainstate::ConnectTip( Ticks(time_3 - time_2), Ticks(m_chainman.time_connect_total), Ticks(m_chainman.time_connect_total) / m_chainman.num_blocks_total); - bool flushed = view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope - assert(flushed); + view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope } const auto time_4{SteadyClock::now()}; m_chainman.time_flush += time_4 - time_3;