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.

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.

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 ?

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

Looks like this was introduced in 421218d, 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?

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:

diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
--- a/src/node/blockstorage.h    (revision 09f004bd9fecbee2216ffb6b7bb787d9ec4f0362)
+++ b/src/node/blockstorage.h    (date 1753416740263)
@@ -52,14 +52,14 @@
 {
 public:
     using CDBWrapper::CDBWrapper;
-    bool WriteBatchSync(const std::vector<std::pair<int, const CBlockFileInfo*>>& fileInfo, int nLastFile, const std::vector<const CBlockIndex*>& blockinfo);
-    bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info);
-    bool ReadLastBlockFile(int& nFile);
-    bool WriteReindexing(bool fReindexing);
+    [[nodiscard]] bool WriteBatchSync(const std::vector<std::pair<int, const CBlockFileInfo*>>& fileInfo, int nLastFile, const std::vector<const CBlockIndex*>& blockinfo);
+    [[nodiscard]] bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info);
+    [[nodiscard]] bool ReadLastBlockFile(int& nFile);
+    [[nodiscard]] bool WriteReindexing(bool fReindexing);
     void ReadReindexing(bool& fReindexing);
-    bool WriteFlag(const std::string& name, bool fValue);
-    bool ReadFlag(const std::string& name, bool& fValue);
-    bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex, const util::SignalInterrupt& interrupt)
+    [[nodiscard]] bool WriteFlag(const std::string& name, bool fValue);
+    [[nodiscard]] bool ReadFlag(const std::string& name, bool& fValue);
+    [[nodiscard]] bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex, const util::SignalInterrupt& interrupt)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 };
 } // namespace kernel

and

diff --git a/src/dbwrapper.h b/src/dbwrapper.h
--- a/src/dbwrapper.h    (revision 09f004bd9fecbee2216ffb6b7bb787d9ec4f0362)
+++ b/src/dbwrapper.h    (date 1753415399087)
@@ -210,7 +210,7 @@
     CDBWrapper& operator=(const CDBWrapper&) = delete;
 
     template <typename K, typename V>
-    bool Read(const K& key, V& value) const
+    [[nodiscard]] bool Read(const K& key, V& value) const
     {
         DataStream ssKey{};
         ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
@@ -230,7 +230,7 @@
     }
 
     template <typename K, typename V>
-    bool Write(const K& key, const V& value, bool fSync = false)
+    [[nodiscard]] bool Write(const K& key, const V& value, bool fSync = false)
     {
         CDBBatch batch(*this);
         batch.Write(key, value);
@@ -255,14 +255,14 @@
     }
 
     template <typename K>
-    bool Erase(const K& key, bool fSync = false)
+    [[nodiscard]] bool Erase(const K& key, bool fSync = false)
     {
         CDBBatch batch(*this);
         batch.Erase(key);
         return WriteBatch(batch, fSync);
     }
 
-    bool WriteBatch(CDBBatch& batch, bool fSync = false);
+    [[nodiscard]] bool WriteBatch(CDBBatch& batch, bool fSync = false);
 
     // Get an estimate of LevelDB memory usage (in bytes).
     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:

void CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync)
{
    // ...
    leveldb::Status status = DBContext().pdb->Write(...);
    if (!status.ok()) { assert(false); } // instead of HandleError(status);
    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.

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.

Looks like this was introduced in 421218d

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?

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

Concept ACK

Implemented the same thing some time ago TheCharlatan@de7e3a9#diff-ac5fbeb5de6f28370bc348a579fc465fe7f7b91df0e0483c6edbf10273278c0d.

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

Rebased after #33116, ready for review again.