validation: abort on DB unreadable coins instead of treating them as missing #34931

furszy commented at 8:22 PM on March 26, 2026: member

Early note: the majority of this PR consists of test coverage. The changes per se are small.

If a UTXO entry on disk can't be deserialized, the node currently treats it as if the coin wouldn't exist instead of aborting with an error. A non-existing coin has a very specific meaning for consensus: any block that spends it would be permanently rejected as invalid
(BLOCK_FAILED_VALID), silently forking the node from the rest of the network. This can't currently be triggered in practice (details below), but it's still the wrong behavior.

The root cause is that CDBWrapper::Read() returns false for both missing entries and
deserialization failures, so CCoinsViewDB::GetCoin() has no way to tell them apart. CCoinsViewErrorCatcher was built to catch database read errors and abort, but it never
fires during deserialization errors because CDBWrapper::Read() swallows the exception before it can propagate. This comment in ExecuteBackedWrapper() spells out the code intent very clearly.

As mentioned initially, this can't happen in practice today. It would require either a bug in the coin serialization path, or a memory corruption before the data reaches LevelDB (at which point we have bigger problems). Random disk-level bit flips are caught earlier by LevelDB's verification (verify_checksums=true, enabled by default), which already propagates correctly as DB_INTERNAL_ERROR. Regardless, a db read issue should never be silently misinterpreted as a consensus violation.

This PR adds CDBWrapper::TryRead(), which returns a ReadStatus that lets callers
discriminate between all possible outcomes. CCoinsViewDB::GetCoin() switches on the result and throws on any error, letting ExecuteBackedWrapper() do what it was designed
to do. CDBWrapper::Read() becomes a thin wrapper over TryRead(), preserving backward
compatibility for all other callers (so we don't have to change non-consensus code here). PeekCoin() is also covered, as it delegates to CCoinsViewDB::GetCoin() at the database
level.

The idea of the PR is to go slowly over the code changes, first commit locks-in the current CDBWrapper::Read() behavior . The second adds TryRead() with tests for all four status codes. The third is the CCoinsViewDB::GetCoin() fix. The fourth is a functional that ensures the node aborts correctly instead of silently diverging.

Testing Notes: Cherry-picking the functional test commit on master demonstrates the consensus split
when the coin entry fails to deserialize.

Extra Note: CDBIterator::GetValue() has the same silent-swallow pattern. Not consensus-critical. Should be addressed in a follow-up.

DrahtBot added the label Validation on Mar 26, 2026

DrahtBot commented at 8:23 PM on March 26, 2026: 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/34931.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	ajtowns
Concept ACK	stickies-v, w0xlt, fjahr, l0rinc
Approach NACK	purpleKarrot

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36042 (build: Bump g++ minimum supported version to 13 by maflcko)
#34812 (net: advertise CJDNS addresses when -externalip disables discovery by w0xlt)
#34132 (coins,dbwrapper: remove error catcher, make point-read failures fatal by l0rinc)

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.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/feature_utxo_abort_on_error.py] assert len(permanently_invalid) == 0, f"Spending block {spending_block_hash} must not be marked BLOCK_FAILED_VALID" -> assert_equal(len(permanently_invalid), 0)

stickies-v commented at 9:20 PM on March 26, 2026: contributor

If a UTXO entry on disk can't be deserialized, the node currently treats it as if the coin wouldn't exist instead of aborting with an error.

Concept ACK

w0xlt commented at 10:33 PM on March 26, 2026: contributor

Concept ACK

fjahr commented at 8:03 AM on March 27, 2026: contributor

Concept ACK

l0rinc commented at 12:48 PM on March 27, 2026: contributor

This was already done in #34132, can you explain why you think we need two competing implementations?

furszy commented at 3:19 PM on March 27, 2026: member

This was already done in #34132, can you explain why you think we need two competing implementations?

We are obviously touching the same code from different angles. Yours from a code-level perspective, and mine from a higher-level consensus divergence scenario. I think it's fairly evident from reading both PR descriptions. I wasn't aware of #34132, but I am now. I find both angles valuable.

From what I can see, your PR is a much broader change focused on removing the CCoinsViewErrorCatcher indirection, and the deserialization fix comes within that larger structural change, so the consensus fix doesn't get the attention/relevance it should have. Mine comes at it from a different direction, solely motivated by the consensus divergence standpoint. A UTXO deserialization failure silently causes BLOCK_FAILED_VALID, permanently forking the node from the network.

The approaches also differ technically. Your #34132 makes Read() itself fatal, changing behavior for all callers at once, which forced you to end up mixing consensus and non-consensus changes together in the same PR. This PR is minimal and much narrower in that sense, it focuses on consensus and nothing else. It adds TryRead() returning an explicit ReadStatus, so GetCoin() can discriminate between a missing key and a deserialization failure without touching Read() for non-consensus callers. All this work started from the functional test bf295eb0af8b8768ce2e659eb8e5afdf1c3a8a5f that exercises the actual silent-fork scenario on master.

I still need to review your PR in detail, but it would be good to focus on what we can do to get the best of both worlds. I think there are valuable changes on both sides. The first point that comes to mind is that the consensus changes and its test coverage should live in its own isolated PR, and then we keep building with broader cleanups and improvements on top. We should avoid mixing them within the same PR. Will dive into your PR to see what else we can combine and improve.

The goal is not to compete, it's to get the best outcome for the codebase.

sedited requested review from theuni on May 8, 2026

sedited requested review from danielabrozzoni on May 8, 2026

sedited requested review from ismaelsadeeq on May 8, 2026

sedited commented at 12:02 PM on May 25, 2026: contributor

Looked at this again with #34132 in mind as extra context. I think I agree that focusing on the consensus paths first is the prudent thing to do. I am not sure of how they could fit together, or what the correct approach really is. I do prefer just aborting early over adding more error conditions, and going back and forth between exceptions and error codes. How about adding a TryRead (or whatever you want to call it) without any error code translation, that just throws directly to the error catcher? Then #34132 can still do the same thing of handling the error earlier by introducing the error callback specifically for this new member function.

sedited commented at 7:38 PM on August 10, 2026: contributor

@furszy how do you want to proceed here?

in src/dbwrapper.h:241 in 4b89e272dd

 237 | +     *         ReadStatus::DESERIALIZATION_ERROR if value deserialization fails,
 238 | +     *         ReadStatus::DB_INTERNAL_ERROR if DB record read fails.
 239 | +     */
 240 |      template <typename K, typename V>
 241 | -    bool Read(const K& key, V& value) const
 242 | +    [[nodiscard]] ReadStatus TryRead(const K& key, V& value) const

ajtowns commented at 5:40 AM on August 24, 2026:

Would something like this be better?

[[nodiscard]] util::Expected<bool, ReadFailure> TryRead(key, value)
{
    ... return false; // not found
    ... return util::Unexpected{DB_INTERNAL_ERROR, e.what()};
    ... return util::Unexpected{DESERIALIZE_ERROR, e.what()};
    ... return true; // success
}

if (const auto found = TryRead(k,v)) {
    if (*found) ... else ...;
} else {
    throw found.error(); // or whatever
}

?

m3dwards commented at 3:45 PM on August 24, 2026:

I don't mind the ReadStatus enum but also agree this could be a case for Expected. However, my expectation when encountering an util::Expected would be for it to include the value not for there to be an out param.

So a signature that looks something like:

// V is listed first so callers write TryRead<Coin>(key) and K is still deduced from `key`.
template <typename V, typename K>
[[nodiscard]] util::Expected<std::optional<V>, ReadFailure> TryRead(const K& key) const

ajtowns commented at 5:30 AM on August 25, 2026:

You'd need to write calls as TryRead<V>(key) to pass V through to specify the deserialization type that way; I don't think it's a win.

furszy commented at 6:34 PM on August 26, 2026:

Applied the util::Expected<bool, ReadFailure> idea. It's a bit wordier than a single switch on ReadStatus, but it may be worthwhile at the API level.

purpleKarrot commented at 8:44 AM on August 25, 2026: contributor

a db read issue should never be silently misinterpreted as a consensus violation.

Strongly agree. Concept ACK.

This PR adds CDBWrapper::TryRead(), which returns a ReadStatus that lets callers discriminate between all possible outcomes.

Strongly disagree. Approach NACK. DB issues are not just another possible outcome that callers should discriminate between. Error information and valid outcomes should preferably not use the same propagation mechanism.

What is wrong with just letting the exception propagate?

furszy commented at 2:42 PM on August 25, 2026: member

Strongly disagree. Approach NACK. DB issues are not just another possible outcome that callers should discriminate between. Error information and valid outcomes should preferably not use the same propagation mechanism.

What is wrong with just letting the exception propagate?

There is nothing wrong with letting exceptions propagate. At least in my view; it really depends on the case, the component being worked on, whether we want to force the caller to handle the error path without letting it bubble up to other components, and whether we expect the caller to be able to resolve or work around the error.

In this specific scenario, while throwing an exception wouldn't be bad, I'm slightly more inclined to force callers to handle the error path in-place to prevent skipping/swallowing the error, as they may want to clean up their state, flush/save what they can to disk, and shutdown the node safely. It should also make #34132 cleanup slightly nicer too.

Of course that this is just my view. If there is consensus on throwing an exception, we can go down that path too. The change is minimal and easily doable. The goal of this PR is to fix the consensus divergence issue, which is just bad. I'm not particularly interested in the broader exceptions vs error return meta discussion. I see benefits in both approaches.

l0rinc commented at 3:26 PM on August 26, 2026: contributor

Error information and valid outcomes should preferably not use the same propagation mechanism.

Functional languages are usually built on this, to avoid having multiple unannounced return values - exceptions are basically weird gotos that are meant to split the happy path from the error handling.

Unfortunately C++20 is not well equipped to flatmap and compose these cases cleanly (though C++23 adds some monadic operations), so error handling is awkward, but exceptions can arguably be even more problematic and should be avoided whenever possible (besides not being announced in the function signature as possible return values, the functions in between get unwound even though they never handle anything, throw cost depends on stack depth so it's unpredictable, and they can't cross thread boundaries at all).

I'd rather keep the functional route here: make the failure part of the return type so it's visible in the signature and the caller is forced to deal with it, even if composing them in C++ is more verbose than it should be.

Concept ACK

test: add missing coverage for CDBWrapper::Read() errors

CDBWrapper::Read() errors have no test coverage or documentation.

Currently, the function can return false when deserialization fails
(indistinguishable from a missing key) and throw dbwrapper_error
on an internal database error.

This commit introduces tests that pin both behaviors so we can work
on improvements in the next commits without worrying about introducing
a behavior change.

f78834fac9

furszy force-pushed on Aug 26, 2026

furszy commented at 6:29 PM on August 27, 2026: member

@furszy how do you want to proceed here?

Updated per feedback @sedited. As said above, unless there is consensus in changing the approach, which wouldn't be a problem for me, I prefer to move forward as is. Introducing test coverage and fixing the bug.

l0rinc commented at 6:37 PM on August 27, 2026: contributor

@furszy: drafted #34132 (comment) to focus on fixing this first.

purpleKarrot commented at 5:51 AM on August 28, 2026: contributor

The goal of this PR is to fix the consensus divergence issue, which is just bad. I'm not particularly interested in the broader exceptions vs error return meta discussion.

I completely agree. Such a discussion is orthogonal to the issue this PR is attempting to fix, so it does not belong here. But a common strategy for error handling is necessary for knowing how this issue should be approached. Since there is no documented common strategy yet, I have opened #36101.

in test/functional/feature_utxo_abort_on_error.py:111 in a72f6acae4

 106 | +
 107 | +        # Confirm node0 aborted with SIGABRT
 108 | +        self.wait_until(lambda: self.nodes[0].is_node_stopped(
 109 | +            expected_ret_code=-6,
 110 | +            expected_stderr="Error: Error reading from database, shutting down.",
 111 | +        ))

maflcko commented at 10:59 AM on August 28, 2026:

                                    unexpected_msgs=["bad-txns-inputs-missingorspent"]):
            try:
                self.connect_nodes(0, 1)
            except Exception:
                pass # node0 may validly abort before connect_nodes returns

            # Confirm node0 aborted with SIGABRT
            self.wait_until(lambda: self.nodes[0].is_node_stopped(
            expected_ret_code=-6,
            expected_stderr="Error: Error reading from database, shutting down.",
            ))

nit: Can avoid the first timeout, by nesting the second wait into the scope.

This should also give a nicer error message on failure about the node failing to stop, as opposed to the debug log not matching

furszy commented at 7:21 PM on August 28, 2026:

Sure, done as suggested.

in src/dbwrapper.h:210 in 5f9e9d863a

 203 | @@ -203,26 +204,82 @@ class CDBWrapper
 204 |      CDBWrapper(const CDBWrapper&) = delete;
 205 |      CDBWrapper& operator=(const CDBWrapper&) = delete;
 206 |  
 207 | +    struct ReadFailure {
 208 | +        enum class Code {
 209 | +            DESERIALIZATION_ERROR,   //!< Key exists but value could not be deserialized.
 210 | +            DB_INTERNAL_ERROR,       //!< Unexpected internal DB error.

ajtowns commented at 11:46 AM on August 28, 2026:

Should be CamelCase ; see developer-notes / #35698

furszy commented at 7:14 PM on August 28, 2026:

Sure, done.

in src/txdb.cpp:77 in 9155635b76 outdated

  75 | -        Assert(!coin.IsSpent()); // The UTXO database should never contain spent coins
  76 | -        return coin;
  77 | +    Coin coin;
  78 | +    const auto ret = m_db->TryRead(CoinEntry(&outpoint), coin);
  79 | +    if (!ret) {
  80 | +        // Propagate errors so CCoinsViewErrorCatcher triggers a clean shutdown.

ajtowns commented at 3:53 PM on August 28, 2026:

I think #34132's approach of dropping CCoinsViewErrorCatcher and dealing with errors directly is better, but fine for that to be a followup.

Might be clearer if written with success paths first?

    Coin coin;
    if (const auto ret = m_db->TryRead(CoinEntry(&outpoint), coin)) {
        if (ret.value()) {
            // Coin found, ensure UTXO database never contains spent coins
            Assert(!coin.IsSpent());
            return coin;
        } else {
            // Coin not found
            return std::nullopt;
        }
    } else {
        // Propagate errors so CCoinsViewErrorCatcher triggers a clean shutdown.
        switch (const auto [err_code, err_msg] = ret.error(); err_code) {
            case CDBWrapper::ReadFailure::Code::DESERIALIZATION_ERROR:
                throw dbwrapper_error{strprintf("Coin deserialization failure: %s", err_msg)};
            case CDBWrapper::ReadFailure::Code::DB_INTERNAL_ERROR:
                throw dbwrapper_error{strprintf("Coin DB read failure: %s", err_msg)};
        } // no default case, so the compiler can warn about missing cases
        std::abort(); // unreachable
    }

furszy commented at 7:21 PM on August 28, 2026:

I wrote it in that way first and wasn't fully convinced about it. The extra nesting seemed slightly harder to follow. But can push that if you are strong on it, no problem.

in src/dbwrapper.h:276 in 5f9e9d863a

 277 | +    template <typename K, typename V>
 278 | +    bool Read(const K& key, V& value) const
 279 | +    {
 280 | +        const auto ret = TryRead(key,value);
 281 | +        if (ret.has_value()) return ret.value();
 282 | +        switch (const auto [err_code, err_msg] = ret.error(); err_code) {

ajtowns commented at 4:06 PM on August 28, 2026:

Should be auto& to capture by ref probably; ditto txdb.

furszy commented at 7:21 PM on August 28, 2026:

Sure, done as suggested.

ajtowns commented at 4:23 PM on August 28, 2026: contributor

ACK. Looks good to me except for the old enum capitalisation.

maflcko commented at 5:13 PM on August 28, 2026: member

.

furszy force-pushed on Aug 28, 2026

furszy commented at 7:30 PM on August 28, 2026: member

Updated per feedback, thanks both for the review.

ajtowns commented at 5:04 AM on August 29, 2026: contributor

Sorry, missed mentioning this earlier: PR description above and the TryRead commit both still talk about ReadStatus rather than Expected<bool,ReadFailure>. Maybe would be better to add using ReadStatus = util::Expected<..> rather than change the descriptions, so that it's easier to talk about? Would also allow writing const ReadStatus res = TryRead(...) for more explicit types?

ACK 443577a64f59c3d6c360274d2276b1bed62b98fc

DrahtBot requested review from l0rinc on Aug 29, 2026

DrahtBot requested review from stickies-v on Aug 29, 2026

DrahtBot requested review from fjahr on Aug 29, 2026

DrahtBot requested review from purpleKarrot on Aug 29, 2026

dbwrapper: add TryRead() to distinguish errors from valid outcomes

Read() returns false for both a missing key and a deserialization
failure, making it impossible for callers to distinguish between
them.

This commits adds TryRead() returning a ReadStatus struct that
discriminates between:

- true:                  record found, value deserialized
- false:                 record not found
- DatabaseError:         levelDB threw during record read
- DeserializationError:  key present, value incompatible with
                         expected format

An err_msg field preserves the original exception message for
diagnostic purposes.

This also makes Read() a thin wrapper over TryRead() to keep
existing call sites unchanged.

Note:
Key serialization is the only operation that may throw in
TryRead(), as callers are expected to provide well-formed keys.
This is why this function is not noexcept.

5dfbb91b6c

txdb: detect UTXO deserialization errors via CDBWrapper::TryRead()

If a UTXO entry on disk can't be deserialized, the node treats it
as if the coin doesn't exist. Any block that spends that coin is
permanently rejected as invalid (BLOCK_FAILED_VALID), silently
forking the node from the rest of the network. This can hardly be
triggered in practice (details below), but it's still the wrong
behavior that could affect us in the future.

The root cause is that CDBWrapper::Read() returns false for both
missing keys and deserialization failures, so the consensus
class CCoinsViewDB::GetCoin() has no way to tell them apart.
CCoinsViewErrorCatcher was built to catch database read errors
and abort, but it never fires because CDBWrapper::Read() swallows
the exception before it can propagate.

In practice, this scenario isn't a latent risk at the moment. It
requires either a bug in the coin serialization path, or memory
corruption before the data reaches LevelDB (at which point we have
bigger problems). Any random disk-level bit flips are caught earlier
by LevelDB's verification (the verify_checksums=true option enabled
by default), which surfaces as a DatabaseError rather than a
deserialization failure.

This commit switches CCoinsViewDB::GetCoin() to use
CDBWrapper::TryRead(), which lets the caller discriminate between
all possible outcomes. On deserialization error, the exception now
propagates through CCoinsViewErrorCatcher to ExecuteBackedWrapper(),
which invokes the shutdown callbacks and aborts the node accordantly.

This also fixes PeekCoin(), which delegates to GetCoin() at the
CCoinsViewDB level.

4652cd0d82

test: exercise node abort on UTXO deserialization failure

This ensures that UTXO unserialization errors abort the node, and does
not cause a consensus divergence.

A valid UTXO is created and shared between two nodes. The raw database
entry is then deliberately modified on one node so it can no longer be
deserialized. When the other node spends that UTXO and mines a block,
the node with the unserializable entry must abort during block connection
rather than silently treating the coin as absent and marking the block
BLOCK_FAILED_VALID, which would cause it to permanently diverge from the
network's best chain.

75f64e50c6

furszy force-pushed on Aug 29, 2026

furszy commented at 3:01 PM on August 29, 2026: member

Maybe would be better to add using ReadStatus = util::Expected<..> rather than change the descriptions, so that it's easier to talk about? Would also allow writing const ReadStatus res = TryRead(...) for more explicit types?

Sure, sounds good. Done as suggested. It is CDBWrapper::ReadStatus rather than only ReadStatus but not a big deal.

ajtowns commented at 7:02 AM on August 30, 2026: contributor

reACK 75f64e50c67dce423efb31fd0a0ac9e1d3320739