refactor: Let CCoinsViewCache::BatchWrite return void

TheCharlatan commented at 9:42 pm on November 12, 2025: contributor

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.

DrahtBot added the label Refactoring on Nov 12, 2025

DrahtBot commented at 9:42 pm on November 12, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33866.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33680 (validation: do not wipe utxo cache for stats/scans/snapshots by l0rinc)
#33602 ([IBD] coins: reduce lookups in dbcache layer propagation by l0rinc)
#33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/coins.h:442 in 6d1818888b

440@@ -441,7 +441,7 @@ class CCoinsViewCache : public CCoinsViewBacked
441      * to be forgotten.
442      * If false is returned, the state of this cache (and its backing view) will be undefined.

l0rinc commented at 10:00 pm on November 12, 2025:

please update the code comments as well:

 0diff --git a/src/coins.h b/src/coins.h
 1--- a/src/coins.h	(revision 75d6ff573ec5b816c78c0df5453d1639c070dc9f)
 2+++ b/src/coins.h	(date 1762988548801)
 3@@ -439,7 +439,6 @@
 4      * Push the modifications applied to this cache to its base and wipe local state.
 5      * Failure to call this method or Sync() before destruction will cause the changes
 6      * to be forgotten.
 7-     * If false is returned, the state of this cache (and its backing view) will be undefined.
 8      */
 9     void Flush();
10 
11@@ -448,7 +447,6 @@
12      * the contents of this cache (except for spent coins, which we erase).
13      * Failure to call this method or Flush() before destruction will cause the changes
14      * to be forgotten.
15-     * If false is returned, the state of this cache (and its backing view) will be undefined.
16      */
17     void Sync();

l0rinc commented at 10:03 pm on November 12, 2025: contributor

big concept ACK, some tests and comments likely still need updating, but can’t wait for this to be finally gone, thanks for taking care of it.

For the record this is a continuation of #33042:

CCoinsView::BatchWrite (and transitively CCoinsViewCache::Flush & CCoinsViewCache::Sync) were intentionally not changed here. While all implementations return true, the base CCoinsView::BatchWrite returns false. Changing this would cause coins_view tests to fail with: terminating due to uncaught exception of type std::logic_error: Not all unspent flagged entries were cleared We can fix that in a follow-up PR.

in src/coins.cpp:19 in 6d1818888b

15@@ -16,7 +16,7 @@ TRACEPOINT_SEMAPHORE(utxocache, uncache);
16 std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; }
17 uint256 CCoinsView::GetBestBlock() const { return uint256(); }
18 std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
19-bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; }
20+void CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { }

l0rinc commented at 10:59 pm on November 12, 2025:

is this still the right abstraction?

andrewtoth commented at 1:43 am on November 13, 2025:

Hmm should we possibly just iterate through the cursor, clearing it? That would make our test code simpler.

l0rinc commented at 2:57 pm on November 13, 2025:

Not exactly what I meant, I don’t think that’s ever called.

My objection was rather that the cache hierarchies have to stop somewhere and we’re currently doing that by adding dummy values at the bottom, e.g.

0CCoinsView viewDummy;
1CCoinsViewCache view(&viewDummy);

But the base field is a pointer, which makes sense, the recursion has to stop somewhere. https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L344 But instead of a nullptr, we’re storing an half-initialized CCoinsView instance which will always return nonsensical values. So now we have two problems, a dummy sentinel and a nullptr both signal the end of recursion. We also have a CCoinsViewBacked which just delegates to the backing cache - except we’re not actually checking for nullptr in most places? https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.cpp#L51

What’s the role of CCoinsViewBacked and CCoinsViewErrorCatcher and the dummy implementation of CCoinsView exactly? We should likely clean this up in a follow-up, but it could help to clarify how the CCoinsView implementations should look like in this one.

l0rinc commented at 3:30 pm on November 13, 2025:

For reference, this is what a pure virtual CCoinsView would look like: l0rinc/bitcoin@d4bec8d (#53)

A lot cleaner, but needs further work and is outside the scope of this change.

in src/test/fuzz/coins_view.cpp:80 in 6d1818888b

73@@ -74,10 +74,23 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
74                 }
75             },
76             [&] {
77-                (void)coins_view_cache.Flush();
78+                try {
79+                    coins_view_cache.Flush();
80+                } catch (const std::logic_error& e) {
81+                    if (is_db || e.what() != std::string{"Not all unspent flagged entries were cleared"}) {

l0rinc commented at 11:05 pm on November 12, 2025:

does this apply to Flush as well?

l0rinc approved

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>

d1823ebdd3

TheCharlatan force-pushed on Nov 13, 2025

TheCharlatan commented at 9:58 am on November 13, 2025: contributor

Updated 6d1818888b1e56dec6c57fadb3a99bfacbab742f -> d1823ebdd32d106668a3a8085da31e274d5c0ac1 (batch_write_void_0 -> batch_write_void_1, compare)

Addressed @andrewtoth’s comment, use NextAndMaybeErase on the cursor in BatchWrite. This avoids the extra code in the fuzz test, and might increase the fuzz test’s ability to generate coverage a bit.
Addressed @l0rinc’s comment, removed left over (and now stale) comments on false returning conditions. Also added as co-author.

in src/coins.h:392 in d1823ebdd3

388@@ -389,7 +389,7 @@ class CCoinsViewCache : public CCoinsViewBacked
389     bool HaveCoin(const COutPoint &outpoint) const override;
390     uint256 GetBestBlock() const override;
391     void SetBestBlock(const uint256 &hashBlock);
392-    bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override;
393+    void BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override;

l0rinc commented at 2:58 pm on November 13, 2025:

How does this change affect the error catchers, e.g. https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L507?

TheCharlatan commented at 11:47 am on November 14, 2025:

I don’t think it does, afaik a write exception through Sync or Flush is just bubbled up and then logged. But I might be missing something.

l0rinc commented at 1:22 pm on November 14, 2025:

but aren’t read and write symmetric now in that regard?

TheCharlatan commented at 1:39 pm on November 14, 2025:

Mmh, not sure what you are trying to say with that. What is the symmetry there? I think I am missing something, can you try and elaborate a bit?

l0rinc commented at 1:45 pm on November 14, 2025:

the comment states: “shutdown on LevelDB read errors […] Writes do not need similar protection”. So now that reads and writes both throw and don’t return, do we need to adjust the error catchers - since either reads also don’t need protection anymore or writes also do - if I understand the catchers’ purpose…

TheCharlatan commented at 1:54 pm on November 14, 2025:

I don’t think we are changing behaviour here (though I might stand corrected still), so if a change is required to them, the error catchers would have been buggy from the beginning. I have long questioned though to how we catch the db write errors, and if it might lead to an infinite loop in some cases. The exception handling just doesn’t seem very robust or thought-through to me.

refactor: Let CCoinsViewCache::BatchWrite return void #33866

Code Coverage & Benchmarks

Reviews

Conflicts