refactor: Let CCoinsViewCache::BatchWrite return void

sedited 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
ACK	l0rinc, andrewtoth, w0xlt, achow101

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34164 (validation: add reusable coins view for ConnectBlock by andrewtoth)
#34125 (refactor: reuse should_empty for chainstate flush condition by l0rinc)
#34124 (refactor: make CCoinsView a purely virtual abstract base class by l0rinc)
#33680 (validation: do not wipe utxo cache for stats/scans/snapshots by l0rinc)
#33512 (coins: use number of dirty cache entries in flush warnings/logs by l0rinc)
#31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)

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.

l0rinc commented at 8:48 am on December 20, 2025:

Pushed the abstraction cleanup in a new PR: #34124 Please resolve this comment

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

sedited force-pushed on Nov 13, 2025

sedited 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 outdated

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?

sedited 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?

sedited 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…

sedited 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.

l0rinc commented at 3:43 pm on December 11, 2025:

I agree that the fundamental behavior didn’t change, both read and write threw instead of returning. Can we adjust the comments at least to reflect this?

l0rinc commented at 9:30 pm on December 14, 2025:

We can also do it in a follow-up, there’s enough work in this area…

sedited commented at 9:35 pm on December 14, 2025:

Yes, I think so too :)

l0rinc commented at 11:02 pm on December 20, 2025:

Pushed in a separate PR: https://github.com/bitcoin/bitcoin/pull/34132/files Please resolve the comment.

DrahtBot added the label Needs rebase on Dec 11, 2025

sedited force-pushed on Dec 14, 2025

sedited commented at 2:01 pm on December 14, 2025: contributor

Rebased d1823ebdd32d106668a3a8085da31e274d5c0ac1 -> d35267f6f887d0c6810b113252c5cc7fd2370373 (batch_write_void_1 -> batch_write_void_2, compare)

Fixed conflict with #33602

DrahtBot removed the label Needs rebase on Dec 14, 2025

andrewtoth approved

andrewtoth commented at 4:05 pm on December 14, 2025: contributor

ACK d35267f6f887d0c6810b113252c5cc7fd2370373

DrahtBot requested review from l0rinc on Dec 14, 2025

in src/coins.h:444 in d35267f6f8

442@@ -443,16 +443,15 @@ class CCoinsViewCache : public CCoinsViewBacked
443      * after flushing and should be destroyed to deallocate.
444      * If false is returned, the state of this cache (and its backing view) will be undefined.

l0rinc commented at 5:10 pm on December 14, 2025:

the comment should also be updated

sedited commented at 6:00 pm on December 14, 2025:

Do you think it would be better if we’d mention the exception too, i.e. If an exception occurs the state of this cache (and its backing view) will be undefined.

l0rinc commented at 6:04 pm on December 14, 2025:

Maybe, I assume it’s evident now that it’s void - the definition of a side-effectful method

in src/coins.cpp:19 in d35267f6f8

15@@ -16,7 +16,11 @@ 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 5:30 pm on December 14, 2025:

the formatting was already off here. Since I think you need to push again to remove an invalid method doc, could you please reformat the modified lines as well?

sedited commented at 5:59 pm on December 14, 2025:

I think I prefer the line break. There is also the longer HaveCoin method below it that also does put everything in one line?

l0rinc commented at 6:01 pm on December 14, 2025:

it’s not the linebreak:

0void CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock)

l0rinc changes_requested

l0rinc commented at 5:34 pm on December 14, 2025: contributor

Recreated the change locally and diffed it against the PR: only the mentioned formatting and doc changes differed.

DrahtBot requested review from l0rinc on Dec 14, 2025

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>

6da6f503a6

sedited force-pushed on Dec 14, 2025

l0rinc commented at 9:29 pm on December 14, 2025: contributor

ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea

Thanks for taking care of this!

DrahtBot requested review from andrewtoth on Dec 14, 2025

sedited commented at 9:34 pm on December 14, 2025: contributor

Updated d35267f6f887d0c6810b113252c5cc7fd2370373 -> 6da6f503a6dd8de85780ca402f5202095aa8b6ea (batch_write_void_2 -> batch_write_void_3, compare)

Addressed @l0rinc’s comment, re-formatted some lines that were already touched.
Addressed @l0rinc’s comment, removed stale docstring for boolean return parameter.

andrewtoth commented at 11:29 pm on December 14, 2025: contributor

re-ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea

l0rinc approved

w0xlt commented at 12:03 pm on December 30, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/33866/commits/6da6f503a6dd8de85780ca402f5202095aa8b6ea

Good cleanup that removes dead code and simplifies the coins cache API.

l0rinc commented at 8:16 pm on January 1, 2026: contributor

A few other PRs depend on this, can we get this one merged? rfm?

achow101 commented at 0:36 am on January 3, 2026: member

ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea

achow101 merged this on Jan 3, 2026

achow101 closed this on Jan 3, 2026

refactor: Let CCoinsViewCache::BatchWrite return void #33866

Code Coverage & Benchmarks

Reviews

Conflicts