refactor: Let CCoinsViewCache::BatchWrite return void #33866

sedited · 2025-11-12T21:42:34Z

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 · 2025-11-12T21:42:46Z

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

src/coins.h

l0rinc · 2025-11-12T22:03:35Z

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.

src/coins.cpp

src/test/fuzz/coins_view.cpp

sedited · 2025-11-13T09:58:53Z

Updated 6d18188 -> d1823eb (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.

l0rinc · 2025-11-13T14:58:47Z

src/coins.h

    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;


How does this change affect the error catchers, e.g.

bitcoin/src/coins.h

Line 507 in dfde31f

* Writes do not need similar protection, as failure to write is handled by the caller.

?

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.

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

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?

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

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.

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?

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

Yes, I think so too :)

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

sedited · 2025-12-14T14:01:12Z

Rebased d1823eb -> d35267f (batch_write_void_1 -> batch_write_void_2, compare)

Fixed conflict with [IBD] coins: reduce lookups in dbcache layer propagation #33602

andrewtoth

ACK d35267f

l0rinc

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

l0rinc · 2025-12-14T17:10:04Z

src/coins.h

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


the comment should also be updated

Suggested change

* If false is returned, the state of this cache (and its backing view) will be undefined.

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.

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

l0rinc · 2025-12-14T17:30:27Z

src/coins.cpp

 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)


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?

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

it's not the linebreak:

Suggested change

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

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

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 <[email protected]>

l0rinc · 2025-12-14T21:29:35Z

ACK 6da6f50

Thanks for taking care of this!

sedited · 2025-12-14T21:34:04Z

Updated d35267f -> 6da6f50 (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 · 2025-12-14T23:29:54Z

re-ACK 6da6f50

w0xlt

ACK 6da6f50

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

l0rinc · 2026-01-01T20:16:27Z

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

achow101 · 2026-01-03T00:36:42Z

ACK 6da6f50

When `CDBWrapper::Read()` fails deserialization we silently return `false`, which makes decode failures indistinguishable from missing keys. Drop the catch so deserialization errors fail fast. This is in line with other recent changes where we're not trying to recover from database failures - and to make this explicit, see: bitcoin#33866

DrahtBot added the Refactoring label Nov 12, 2025

l0rinc reviewed Nov 12, 2025

View reviewed changes

src/coins.h Outdated Show resolved Hide resolved

l0rinc approved these changes Nov 12, 2025

View reviewed changes

src/coins.cpp Outdated Show resolved Hide resolved

src/test/fuzz/coins_view.cpp Outdated Show resolved Hide resolved

andrewtoth mentioned this pull request Nov 13, 2025

coins: use number of dirty cache entries in flush warnings/logs #33512

Open

sedited force-pushed the batch_write_void branch from 6d18188 to d1823eb Compare November 13, 2025 09:58

l0rinc reviewed Nov 13, 2025

View reviewed changes

DrahtBot mentioned this pull request Nov 24, 2025

validation: fetch block inputs on parallel threads 3x faster IBD #31132

Open

2 tasks

DrahtBot added the Needs rebase label Dec 11, 2025

sedited force-pushed the batch_write_void branch from d1823eb to d35267f Compare December 14, 2025 13:59

DrahtBot removed the Needs rebase label Dec 14, 2025

andrewtoth approved these changes Dec 14, 2025

View reviewed changes

DrahtBot requested a review from l0rinc December 14, 2025 16:05

l0rinc suggested changes Dec 14, 2025

View reviewed changes

DrahtBot requested a review from l0rinc December 14, 2025 17:34

sedited force-pushed the batch_write_void branch from d35267f to 6da6f50 Compare December 14, 2025 21:27

DrahtBot requested a review from andrewtoth December 14, 2025 21:29

l0rinc approved these changes Dec 15, 2025

View reviewed changes

l0rinc mentioned this pull request Dec 19, 2025

refactor: inline CCoinsViewErrorCatcher into CCoinsViewDB l0rinc/bitcoin#72

Closed

DrahtBot mentioned this pull request Dec 29, 2025

validation: add reusable coins view for ConnectBlock #34164

Open

w0xlt reviewed Dec 30, 2025

View reviewed changes

achow101 merged commit ab23325 into bitcoin:master Jan 3, 2026
25 checks passed

kevkevinpal mentioned this pull request Jan 3, 2026

refactor: Let CCoinsViewCache::BatchWrite return void kevkevinpal/bitcoin#238

Open

This was referenced Jan 8, 2026

chacha20 l0rinc/bitcoin#88

Draft

Coins Cache Cleanup checklist #34280

Open

l0rinc mentioned this pull request Jan 15, 2026

coins: replace remaining HaveCoin calls with GetCoin l0rinc/bitcoin#98

Draft

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

refactor: Let CCoinsViewCache::BatchWrite return void #33866

refactor: Let CCoinsViewCache::BatchWrite return void #33866

Uh oh!

Conversation

sedited commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

l0rinc commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedited commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited commented Dec 14, 2025

Uh oh!

andrewtoth left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Dec 14, 2025

Uh oh!

sedited commented Dec 14, 2025

Uh oh!

andrewtoth commented Dec 14, 2025

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Jan 1, 2026

Uh oh!

achow101 commented Jan 3, 2026

Uh oh!

Uh oh!

Reviewers

sedited commented Nov 12, 2025 •

edited

Loading

DrahtBot commented Nov 12, 2025 •

edited

Loading

l0rinc commented Nov 12, 2025 •

edited

Loading

l0rinc Dec 14, 2025 •

edited

Loading

l0rinc Dec 14, 2025 •

edited

Loading