#34132 coins, dbwrapper: remove error catcher, make point-read failures fatal
https://github.com/bitcoin/bitcoin/pull/34132 · · +292/-381 in 33 files, 9 commits · labels: UTXO Db and Indexes
Goal
- Prevent database read failures and corruption from masquerading as missing coins
- Safely shut down the node instead of running with corrupt chainstate or rejecting valid transactions
- Simplify the coin view stack by eliminating the dedicated error catcher layer
Centralizes LevelDB point-read and deserialization error handling in `CDBWrapper`, routing failures to a fatal notification callback and `std::abort()`. Removes the redundant `CCoinsViewErrorCatcher` layer, restricts `HaveCoin()` to `CCoinsViewCache`, and annotates coin view lookups as `noexcept`.
Problem: Previously, deserialization errors during `CDBWrapper::Read()` returned `false` while LevelDB read errors threw exceptions. In chainstate and indexes, this could mask database corruption as non-existent entries (e.g., falsely failing valid transactions as missing inputs) or cause unhandled exceptions, requiring the specialized `CCoinsViewErrorCatcher` wrapper solely to catch errors and abort.
Category: Utilities (logging, arguments, libraries) (#2 of 66)
P2 · bug fix
- CDBWrapper previously treated deserialization failures as missing entries, potentially masking disk or memory corruption across any subsystem using CDBWrapper. Standardizing failure-to-abort handling at the database wrapper level prevents silent data corruption bugs.
CDBWrapper previously treated deserialization failures as missing entries, potentially masking disk or memory corruption across any subsystem using CDBWrapper. Standardizing failure-to-abort handling at the database wrapper level prevents silent data corruption bugs.
Membership: Modifies CDBWrapper in src/dbwrapper.cpp and src/dbwrapper.h to unify LevelDB read and deserialization error handling.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 1
Category: Validation (#2 of 48)
P2 · cleanup
- P2 because it prevents corrupt database entries from masquerading as missing inputs during validation
- Avoids silent node operation with corrupt state that could reject valid blocks and transactions
- Simplifies coin view lookups across chainstate and index databases
Directly impacts how UTXO read errors are handled during validation. Removing CCoinsViewErrorCatcher simplifies the coins view hierarchy, avoids mistaking unreadable UTXOs for missing coins during block or tx validation, and marks coin lookups noexcept.
Membership: Modifies chainstate coin access views, removes CCoinsViewErrorCatcher, and touches src/txdb.cpp and src/coins.cpp.
Factors: security/stability 2, bug 2, performance 1, user value 1, leverage 2
Reviewability: Ready
- Ready to review
- Clean state and CI passes after recent rebase
The PR is clean, passing CI, and author recently rebased and addressed dependencies.
Author status: Active; rebased on master after #34931 on 2026-09-12 and invited ajtowns to discuss index error handling on IRC.
Open concerns:
- ajtowns questioned whether optional index read failures (e.g., txindex) should abort the whole node rather than disabling the index, and noted the callback chain to GUI message box before abort is complex.
Resolved concerns:
- Functional test failures on 32-bit systems (due to LevelDB mmap vs block cache behavior differences) were resolved by l0rinc after analysis with maflcko.
- Handling of `HaveCoin` was resolved by retaining it only on `CCoinsViewCache` as suggested by optout21.
Agreement: Mild
- General agreement on treating database read errors as fatal and simplifying the view stack
- Approval of code and test coverage (optout21)
- Concept approval for removing indirection (andrewtoth, sedited)
- Open concern whether corrupt optional indexes should crash the whole node (ajtowns)
Mild: ajtowns raised concerns over aborting on optional index corruption; author suggested discussing on IRC.
Several contributors support the simplification and crash-on-corruption semantics, but ajtowns has an open architectural critique regarding whether optional indexes should crash the entire node.
- andrewtoth: 'I think getting rid of this indirection is great. It reduces a lot of complexity when thinking about the stack of views.'
- ajtowns: 'The UTXO database isn't optional for running a node, all the indexes are... crashing on corruption in that would probably make sense too'
- l0rinc: 'Optional-index point-read failures still abort the node, as in the previous version... @ajtowns, should we discuss this at the next IRC?'
- optout21: 'reACK 6c07101f28ade1a3023d4fe15042ce6b8c97ffa8'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| ajtowns | approach | Aborting the entire node on optional index corruption takes down operating nodes (e.g. lightning node) for non-essential failures, and GUI fatal error callback before abort is convoluted. | open | no | yes | 2026-08-24: 'doing that on a corrupt index seems a bit more questionable -- if the txindex or blockfilter or coinstats indexes fail, I could see it being better to just disable the index, but leave the node operating otherwise' |
| maflcko | correctness | swallowing deserialization exceptions or catching them without aborting leaves program continuing despite storage corruption or severe logic bugs | resolved | no | yes | 2026-02-02: 'When running valgrind ... and calling gettxoutsetinfo I get: DataStream::read(): end of data (code -1) Which still seems wrong for several reasons: Because the exception message is mostly meaningless in this context. The program continues, even though there is a clear storage corruption or severe logic bug' Settled: 2026-02-11: l0rinc restructured the PR to thread fatal read handling into CDBWrapper: 'Instead of catching exceptions at individual call sites just to run a shutdown callback, it threads a fatal read_error_cb down into CDBWrapper via DBParams, so all Read() users share the same behavior on corruption.' |
Support:
- andrewtoth: Reduces view stack indirection and complexity.
- sedited: Concept ACK. [not substantive]
- optout21: Code review and local unit tests OK.
Participants: andrewtoth (support), sedited (support), maflcko (objection), ajtowns (objection), optout21 (support)
State derived from the lists: nonblocking objection open (ajtowns)
Review verdicts (DrahtBot): 1 (+1)
- ACK: optout21
- Stale ACK: andrewtoth
- Concept ACK: sedited
Files
File list not available for this run.
Card
This PR modifies CDBWrapper to handle LevelDB point-read and typed deserialization failures by logging, firing a fatal notification callback, and aborting. It removes the CCoinsViewErrorCatcher wrapper layer, marks coin view lookups noexcept, and confines HaveCoin to CCoinsViewCache. It resolves a long-standing risk where corrupt database entries could masquerade as non-existent keys. While multiple reviewers ACK the cleanup, ajtowns raised an open non-blocking concern that aborting on optional index corruption is overly drastic.