refactor: inline constant return values from `dbwrapper` write methods

l0rinc commented at 2:14 am on July 23, 2025: contributor

Summary

WriteBatch always returns true - the errors are handled by throwing dbwrapper_error instead.

Context

This boolean return value of the Write methods is confusing because it’s inconsistent with CDBWrapper::Read, which catches exceptions and returns a boolean to indicate success/failure. It’s bad that Read returns and Write throws - but it’s a lot worse that Write advertises a return value when it actually communicates errors through exceptions.

Solution

This PR removes the constant return values from write methods and inlines true at their call sites. Many upstream methods had boolean return values only because they were propagating these constants - those have been cleaned up as well.

Methods that returned a constant true value that now return void:

CDBWrapper::WriteBatch, CDBWrapper::Write, CDBWrapper::Erase
TxIndex::DB::WriteTxs
BlockTreeDB::WriteReindexing, BlockTreeDB::WriteBatchSync, BlockTreeDB::WriteFlag
BlockManager::WriteBlockIndexDB

Note

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.

DrahtBot added the label Refactoring on Jul 23, 2025

DrahtBot commented at 2:14 am on July 23, 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/33042.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84
Concept ACK	TheCharlatan

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:

#32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by TheCharlatan)
#30469 (index: Fix coinstats overflow by fjahr)

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.

janb84 commented at 2:07 pm on July 23, 2025: contributor

Is this refactor also part of a series to align the CDBWrapper::Read with CDBWrapper::write e.g. also converting that to return type void and communicating errors through exceptions ?

l0rinc commented at 3:41 pm on July 23, 2025: contributor

We could go the other way as well, enabling writes to only return a boolean and handle them everywhere. I’m open to suggestions.

maflcko commented at 10:52 am on July 24, 2025: member

Looks like this was introduced in 421218d3040279c84616891e8d14b05576b07c57, I guess because it was less verbose. How verbose would it be to pass it up and check it everywhere? Also, git grep '\<catch\>' ./src/index/base.* doesn’t return anything, so I wonder how index code deals with those exceptions?

l0rinc commented at 5:46 am on July 25, 2025: contributor

How verbose would it be to pass it up and check it everywhere?

With CDBWrapper::Read all callers are trying to handle the failure (usually by returning false or std::nullopt or std::vector<uint256>() or resetting the state or manually logging a failure). I have tried unifying Write and Read via:

 0diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
 1--- a/src/node/blockstorage.h    (revision 09f004bd9fecbee2216ffb6b7bb787d9ec4f0362)
 2+++ b/src/node/blockstorage.h    (date 1753416740263)
 3@@ -52,14 +52,14 @@
 4 {
 5 public:
 6     using CDBWrapper::CDBWrapper;
 7-    bool WriteBatchSync(const std::vector<std::pair<int, const CBlockFileInfo*>>& fileInfo, int nLastFile, const std::vector<const CBlockIndex*>& blockinfo);
 8-    bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info);
 9-    bool ReadLastBlockFile(int& nFile);
10-    bool WriteReindexing(bool fReindexing);
11+    [[nodiscard]] bool WriteBatchSync(const std::vector<std::pair<int, const CBlockFileInfo*>>& fileInfo, int nLastFile, const std::vector<const CBlockIndex*>& blockinfo);
12+    [[nodiscard]] bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info);
13+    [[nodiscard]] bool ReadLastBlockFile(int& nFile);
14+    [[nodiscard]] bool WriteReindexing(bool fReindexing);
15     void ReadReindexing(bool& fReindexing);
16-    bool WriteFlag(const std::string& name, bool fValue);
17-    bool ReadFlag(const std::string& name, bool& fValue);
18-    bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex, const util::SignalInterrupt& interrupt)
19+    [[nodiscard]] bool WriteFlag(const std::string& name, bool fValue);
20+    [[nodiscard]] bool ReadFlag(const std::string& name, bool& fValue);
21+    [[nodiscard]] bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex, const util::SignalInterrupt& interrupt)
22         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
23 };
24 } // namespace kernel

and

 0diff --git a/src/dbwrapper.h b/src/dbwrapper.h
 1--- a/src/dbwrapper.h    (revision 09f004bd9fecbee2216ffb6b7bb787d9ec4f0362)
 2+++ b/src/dbwrapper.h    (date 1753415399087)
 3@@ -210,7 +210,7 @@
 4     CDBWrapper& operator=(const CDBWrapper&) = delete;
 5 
 6     template <typename K, typename V>
 7-    bool Read(const K& key, V& value) const
 8+    [[nodiscard]] bool Read(const K& key, V& value) const
 9     {
10         DataStream ssKey{};
11         ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
12@@ -230,7 +230,7 @@
13     }
14 
15     template <typename K, typename V>
16-    bool Write(const K& key, const V& value, bool fSync = false)
17+    [[nodiscard]] bool Write(const K& key, const V& value, bool fSync = false)
18     {
19         CDBBatch batch(*this);
20         batch.Write(key, value);
21@@ -255,14 +255,14 @@
22     }
23 
24     template <typename K>
25-    bool Erase(const K& key, bool fSync = false)
26+    [[nodiscard]] bool Erase(const K& key, bool fSync = false)
27     {
28         CDBBatch batch(*this);
29         batch.Erase(key);
30         return WriteBatch(batch, fSync);
31     }
32 
33-    bool WriteBatch(CDBBatch& batch, bool fSync = false);
34+    [[nodiscard]] bool WriteBatch(CDBBatch& batch, bool fSync = false);
35 
36     // Get an estimate of LevelDB memory usage (in bytes).
37     size_t DynamicMemoryUsage() const;

and handling all of the calls, but I’m getting failures all over the place, mostly because Read cannot tell the difference between missing values and errors. I’ll investigate further.

I wonder how index code deals with those exceptions

Checking all index usages of CDBWrapper::WriteBatch, I didn’t see a single case where we’re trying to handle any exception - seems we assumed the return value reflects the status. One of the usages outside of indexes where the exception would be cought is https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2858.

Also, it seem the build passes with:

0void CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync)
1{
2    // ...
3    leveldb::Status status = DBContext().pdb->Write(...);
4    if (!status.ok()) { assert(false); } // instead of HandleError(status);
5    if (!status.ok()) { } // alternatively, this also passes

So we don’t really have coverage for errors, even though DBImpl::Write has several legitimate reason to fail (especially in LevelDB paranoid mode). https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_init.py#L181-L184 is the only test I found which reproduces any HandleError failure - maybe we can extend that to cover WriteBatch as well.

maflcko commented at 6:15 am on July 25, 2025: member

and handling all of the calls, but I’m getting failures all over the place, mostly because Read cannot tell the difference between missing values and errors. I’ll investigate further.

Yeah, I’d presume you’d have to use Result<bool>, which I’d suspect would be highly verbose.

I didn’t see a single case where we’re trying to handle any exception - seems we assumed the return value reflects the status.

I tried that yesterday, and an error in the index code (scheduler thread, or the sync thread via TraceThread) will just crash the program, IIRC.

l0rinc commented at 0:50 am on July 26, 2025: contributor

Looks like this was introduced in https://github.com/bitcoin/bitcoin/commit/421218d3040279c84616891e8d14b05576b07c57

Maybe @sipa can help us out here: should we try making read/write symmetric (either by write returning, or read throwing) or is the current refactor the cleanest solution for maybe-throwing-while-always-returning-true?

maflcko commented at 10:31 am on July 28, 2025: member

I think the question boils down to: How much UX do we want to provide for data corruption or internal bugs? A valid answer could be: None. In that case you can just use Assert to ensure no data corruption or internal bugs happen. (If they do, the program terminates, like with other Asserts that unexpectedly hit in production.) This approach will likely use the least amount of code.

Other valid answers are: Best-effort (use throw, and maybe catch it, or not). Full-effort (use Result).

TheCharlatan commented at 4:22 pm on July 31, 2025: contributor

Concept ACK

Implemented the same thing some time ago https://github.com/TheCharlatan/bitcoin/commit/de7e3a973ef5773714cd15ead499fbf734547cf1#diff-ac5fbeb5de6f28370bc348a579fc465fe7f7b91df0e0483c6edbf10273278c0d.

l0rinc commented at 4:39 pm on July 31, 2025: contributor

The IRC meeting contained feedback on the overall direction:

<achow101> #topic dbwrapper read/write asymmetry (l0rinc) <l0rinc> dbwrapper writes currently pretend to return bool but always return true, while real errors surface via exceptions – #33042 makes that explicit, but we need to decide the canonical error path <l0rinc> read catches the exception and returns, but write always returned true, while throwing in the background <achow101> how do errors currently propagate? crashing everything? <l0rinc> most of the time yes <TheCharlatan> they are eventually converted to FatalError’s yeah <l0rinc> is the removal of the fake return value enough here or do we need to unify the read/write interfaces? <achow101> if we can’t write state to disk, that does seem like a situation where we’d want to abort anyways <achow101> if the return value is never used, then we should not return it <TheCharlatan> rewriting it to propagate an error code seems riskier than just removing what is essentially dead code. <l0rinc> thanks for the feedback, I’ll add these comments to the PR - unless the authors want to <darosior> TheCharlatan: +1

DrahtBot added the label Needs rebase on Aug 13, 2025

refactor: inline constant return value of `CDBWrapper::WriteBatch`

`WriteBatch` can only ever return `true` - its errors are handled by throwing a `throw dbwrapper_error` instead.
The boolean return value is quite confusing, especially since it's symmetric with `CDBWrapper::Read`, which catches the exceptions and returns a boolean instead.
We're removing the constant return value and inlining `true` for its usages.

50b63a5698

refactor: inline constant return value of `TxIndex::DB::WriteTxs` d1847cf5b5

refactor: inline constant return value of `CDBWrapper::Write` cdab9480e9

refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing`

Did both in this commit, since the return value of `WriteReindexing` was ignored anyway - which existed only because of the constant `Erase` being called

e030240e90

refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` 743abbcbde

l0rinc force-pushed on Aug 13, 2025

l0rinc commented at 10:50 pm on August 13, 2025: contributor

Rebased after #33116, ready for review again.

DrahtBot removed the label Needs rebase on Aug 13, 2025

janb84 commented at 9:28 am on August 14, 2025: contributor

So after reading the feedback from the IRC discussion, in my opinion the PR should be fine. (Not sure about the dead code part and if there is still the need to delete some dead code but that can be a follow up PR )

This PR changes the signature of the function to set the return value to void. This to reflect the inner workings of the function better. The function does not return a relevant bool but always returns true. This is now corrected.

ACK 743abbcbde9e5a2db489bca461c98df461eff7d0

code review ✅
build & tested ✅

DrahtBot requested review from TheCharlatan on Aug 14, 2025

refactor: inline constant return values from dbwrapper write methods #33042