Merge bitcoin/bitcoin#33866: refactor: Let CCoinsViewCache::BatchWrite return void

6da6f503a6 refactor: Let CCoinsViewCache::BatchWrite return void (TheCharlatan)

Pull request description:

  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.

ACKs for top commit:
  l0rinc:
    ACK 6da6f503a6
  andrewtoth:
    re-ACK 6da6f503a6
  achow101:
    ACK 6da6f503a6
  w0xlt:
    ACK 6da6f503a6

Tree-SHA512: dfaa325b0cf8108910aebf1b27434aaddb639d10d860e96797c77ea42eca9035a54a7dc1d6a5d4eae2b75fcc9356206d3d5672243d2c906e80d19024c8b95408

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

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)};

src/test/fuzz/coinscache_sim.cpp

@@ -163,7 +163,7 @@ public:
     std::unique_ptr<CCoinsViewCursor> 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;
     }
 };
 

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);
 }