← Watch feed
refactor: Let CCoinsViewCache::BatchWrite return void
Public commit record
95/100 · Strong
What the developer wrote
Authored by TheCharlatan
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 <pap.lorinc@gmail.com>
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Uses a recognizable type or scope✓ Provides detailed explanatory context✓ Mentions testing or verification
What changed, and why it matters
This is a code cleanup change in Bitcoin Core. It changes several internal coin-cache flushing functions from returning true/false to returning nothing, because in normal use they always succeeded anyway. It removes some now-unnecessary error checks and updates tests to match. There is no direct security bug being fixed here; it is a refactoring that simplifies the code and removes a theoretically reachable but practically dead error path.
Recommended action
No immediate action required. Treat as routine refactoring. Reviewers should verify that no real failure mode was silently removed (e.g., database write failures are still handled elsewhere, such as in CDBWrapper exceptions or leveldb error paths), and that test coverage remains adequate for the new void-return API.
Security signals we found
01
Removal of a previously reachable error-return path that could trigger FatalError in validation flush logic
02
Change from boolean success signaling to void contract in UTXO cache persistence layer
03
Base CCoinsView::BatchWrite stub now clears cursor instead of returning false
04
Asserts and FatalError branches removed because the failure mode is no longer representable
Evidence from the diff
The commit refactors CCoinsViewCache::BatchWrite, CCoinsViewCache::Flush, CCoinsViewCache::Sync, and related CCoinsView/CCoinsViewBacked/CCoinsViewDB overrides to return void instead of bool. Previously these methods always returned true in production; the only false return came from the base CCoinsView::BatchWrite stub, which is not used by backed caches. The patch makes the base stub iterate and clear the cursor instead of returning false, and removes FatalError handling in Chainstate::FlushStateToDisk and assert checks in ConnectTip/DisconnectTip. This eliminates a dead error path and simplifies state management. The change is behavior-preserving for production code because the previous ‘error’ path was unreachable.
Changed components
src/coins.cppsrc/coins.hsrc/txdb.cppsrc/txdb.hsrc/validation.cppsrc/test/coins_tests.cppsrc/test/fuzz/coins_view.cppsrc/test/fuzz/coinscache_sim.cppsrc/test/validation_flush_tests.cppInspect captured patch +41 / −50
diff --git a/src/coins.cpp b/src/coins.cpp
index 1ca5fca3..1a0a8f80 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -16,7 +16,11 @@ TRACEPOINT_SEMAPHORE(utxocache, uncache);
std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; }
uint256 CCoinsView::GetBestBlock() const { return uint256(); }
std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
-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<CCoinsViewCursor> 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<uint256> 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<CCoinsViewCursor> 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 8d07f7fe..6da53829 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<CCoinsViewCursor> Cursor() const;
@@ -350,7 +350,7 @@ public:
uint256 GetBestBlock() const override;
std::vector<uint256> 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<CCoinsViewCursor> 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<CCoinsViewCursor> 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 a1152d24..e07164da 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 9990e9cb..09595678 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 82e5f1fc..e2ca09a7 100644
--- a/src/test/fuzz/coinscache_sim.cpp
+++ b/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;
}
};
diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp
index 2477cb19..66c284b9 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 843cb9b1..abe76c44 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -90,7 +90,8 @@ std::vector<uint256> 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 ea0cf9d7..272db9ca 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<uint256> GetHeadBlocks() const override;
- bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override;
+ void BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override;
std::unique_ptr<CCoinsViewCursor> Cursor() const override;
//! Whether an unsupported database format is used.
diff --git a/src/validation.cpp b/src/validation.cpp
index 3464e588..87bc6410 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<std::chrono::microseconds>(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<MillisecondsDouble>(SteadyClock::now() - time_start));
@@ -3101,8 +3098,7 @@ bool Chainstate::ConnectTip(
Ticks<MillisecondsDouble>(time_3 - time_2),
Ticks<SecondsDouble>(m_chainman.time_connect_total),
Ticks<MillisecondsDouble>(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;
Risk score
Our methodology →Why this scored 18/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.