#34132 coins, dbwrapper: remove error catcher, make point-read failures fatal

ReviewabilityReviewsAgreementSize
#34132 l0rinc coins, dbwrapper: remove error catcher, make point-read failures fatalReady0 (+2)Blocked139 + 153 tests
  • Prevent nodes from continuing to run with damaged state when database reads encounter corruption
  • Stop treating corrupted records as merely missing keys during coin and index lookups
  • Ready for review
  • Unaddressed objection: aborting in DB reads breaks startup recovery and kills kernel hosts (furszy)
  • Unaddressed objection: aborting on optional index corruption halts nodes for non-critical errors (ajtowns)
  • Unaddressed objection: thread safety of the GUI message box callback before abort is unclear (ajtowns)
  • Support for removing indirection in the coin view stack (andrewtoth)
  • Verified with code review and local unit tests (optout21)
  • 139 lines added or modified outside tests
  • 153 lines added or modified in tests
  • 381 lines removed in total
  • 33 files, 9 commits
Indexes#3 of 11
P2 · bug fix
  • P2 because corrupted index records previously caused isolated RPC failures while nodes ran damaged
  • Failing fast on index corruption prevents nodes from continuing with silently damaged index state
Ranking notes
Validation#10 of 53
P2 · bug fix
  • P2 because deserialization failures in the UTXO database were treated as missing keys rather than errors
  • Ensuring corrupt coin reads fail safely protects chainstate integrity across validation lookups
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Centralizes LevelDB point-read and deserialization error handling directly in CDBWrapper by logging errors, invoking a registered fatal-error notification callback, and aborting. Removes the CCoinsViewErrorCatcher view wrapper layer, wiring CCoinsViewCache directly to CCoinsViewDB. Confines HaveCoin() to CCoinsViewCache and marks coin view lookup methods noexcept.

Problem: CDBWrapper::Read() previously returned false on deserialization failures—making corrupted data appear as non-existent keys—while LevelDB point-read failures threw exceptions without invoking fatal error notifications. Consequently, corrupted index records could fail individual RPC calls while allowing the node to continue running with damaged state, and chainstate required a dedicated CCoinsViewErrorCatcher wrapper layer solely to intercept read errors.

Evidence of importance:

  • PR description: 'A corrupt txindex point read can consequently fail one RPC while the node continues running.'
  • Commit c916a71961: 'When a txindex point read encounters a corrupt LevelDB table, the RPC reports the error and the node continues running. CDBWrapper::Read() throws on a LevelDB failure without running the read-error callback and reports a corrupt value as a missing key.'

Scope: Changes error-handling semantics across CDBWrapper, chainstate UTXO database lookups, and all secondary indexes (txindex, coinstatsindex, blockfilterindex, and txospenderindex).

Risk of a mistake: medium (validation). A flaw in database read error classification could trigger unexpected process aborts during normal coin or index lookups, halting the node.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • author: address furszy's objection that aborting in db read breaks startup recovery and kernel library usage 2026-10-29

Agreement

Blocked

objection that needs a change, nobody has replied (ajtowns, furszy)

Thread notes: Added furszy to participants with an objection regarding fatal DB read aborts breaking startup recovery and killing kernel library hosts. Ignored the inline style nit rc:4401858796.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
andrewtoth (member)question, correctnessWhy does the coinsdb read error test fail without IPC on 32-bit systems?answered · author investigated and explained LevelDB disables mmap on 32-bit architectures, causing stale block cache readsansweredyes2026-01-04: 'Do we know why?'
Status from: 2026-01-11: l0rinc: 'The trick was that on 64-bit systems, LevelDB uses mmap which reflects file changes immediately - this is what we we're '
andrewtoth (member)suggestion, correctnessAdd verification, such as asserting a debug log, that the read error callback is executed in the functional test.accepted · andrewtoth withdrew the suggestionansweredyes2026-01-11: 'Is there a way to verify that this callback is still being executed in the functional test? Maybe add a debug log that we can assert?'
Status from: 2026-01-11: andrewtoth: 'Right nvm, LGTM.'
maflcko (member)suggestion, approachKeep ExecuteBackedWrapper and move it to txdb.cpp instead of removing it.answered · author declined, preferring simple local try-catch and noting HaveCoin removalansweredyes2026-01-13: 'I think the ExecuteBackedWrapper can be kept. It seems applicable if it is moved to txdb.cpp as well?'
Status from: 2026-01-24: l0rinc: 'It's a simple try-catch, I don't see the point in extending the call stack even more, we don't do that in other cases an'
andrewtoth (member)suggestion, approachKeep an optimized existence check on the database backing view that avoids copying, caching, or deserialization.answered · author explained Exists still performs a DB Get and folded #34320 keeping HaveCoin only on CCoinsViewCacheansweredyes2026-01-24: 'I think having an optimized existence check that doesn't make copies, cache the value or deserialize after disk lookup makes sense to keep.'
Status from: 2026-08-11: l0rinc: 'following @optout21's suggestion there, HaveCoin() now remains only on CCoinsViewCache'
maflcko (member)objection, safetySwallowing deserialization exceptions or catching them without aborting leaves the program continuing despite storage corruption or severe logic bugs.fixed · fix pushed · CDBWrapper::Read() was changed to treat deserialization failures as fatal via FatalReadError()changedyes2026-02-02: 'The program continues, even though there is a clear storage corruption or severe logic bug'
Status from: 2026-02-11: l0rinc: 'CDBWrapper::Read() now catches deserialization exceptions and calls FatalReadError() which does LogError -> m_read_error'
ajtowns (member)objection, approachAborting the entire node on optional index corruption takes down operating nodes for non-essential failures.contested · ajtowns pushed back against taking down operating nodes on optional index corruption; author maintained fatal behavior in c:5643408398 and proposed IRC discussionchangedyes2026-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'
Status from: 2026-08-24: ajtowns: 'If I'm running a node in order to receive funds via lightning or btcpayserver and enabled an index because I thought it '
ajtowns (member)objection, maintenanceThe callback chain to the GUI message box before abort is convoluted and difficult to trace thread safety for across threads and locks.openchangedyes2026-08-24: 'Tracing through callback logic safety ("It must be safe to call from any thread, with or without locks held") isn't very straightforward, so it would be nice to not have to do that.'
furszy (member)objection, correctnessAborting abruptly inside DB read functions breaks startup automatic recovery paths and kills host applications using the kernel library during init.openchangedno2026-10-29: 'Where a chainstate read failure triggers an automatic recovery process (which should be broken on this branch).'

Support:

  • andrewtoth (member) (Concept ACK, 2026-01-04) [read]: I think getting rid of this indirection is great. It reduces a lot of complexity when thinking about the stack of views.
  • sedited (member) (Concept ACK, 2026-01-05): Concept ACK [not substantive]
  • andrewtoth (member) (ACK, 2026-01-11): ACK 4ae1f793d6718edf7a94fb032a2a7423105e85bd [not substantive]
  • optout21 (contributor) (ACK, 2026-09-04) [both]: Code review, local unit tests OK! [not substantive]
  • optout21 (contributor) (ACK, 2026-09-14): reACK 6c07101f28ade1a3023d4fe15042ce6b8c97ffa8 [not substantive]

Participants:

  • andrewtoth (member, support, 12 statements 2026-01-04 to 2026-01-29): Concept ACK and ACKed an earlier commit; discussed HaveCoin implementation details and functional test verification.
  • sedited (member, support, 1 statements 2026-01-05 to 2026-01-05): Concept ACK.
  • maflcko (member, objection, 17 statements 2026-01-06 to 2026-02-10): Objected to continuing execution on storage corruption or deserialization errors, advocating wholesale fatal aborts.
  • ajtowns (member, objection, 3 statements 2026-08-24 to 2026-08-25): Objected to aborting the entire node on optional index corruption and questioned the convoluted callback architecture.
  • optout21 (contributor, support, 2 statements 2026-09-04 to 2026-09-14): ACK and reACK after testing and code review.
  • furszy (member, objection, 2 statements 2026-10-28 to 2026-10-29): Objected to aborting inside DB read functions, noting it breaks startup recovery paths and abruptly terminates kernel library hosts.

Files

255 lines under test/bench/ci.

  • src/dbwrapper.h +26/-73
  • src/test/dbwrapper_tests.cpp +13/-65
  • test/functional/feature_coinsdb_read_error.py +77/-0
  • src/test/coins_tests.cpp +50/-12
  • src/coins.h +16/-45
  • src/coins.cpp +9/-39
  • src/txdb.cpp +6/-25
  • src/init.cpp +15/-12
  • src/dbwrapper.cpp +10/-15
  • src/index/txindex.cpp +12/-9
  • src/index/blockfilterindex.cpp +6/-13
  • src/index/coinstatsindex.cpp +4/-12
  • src/validation.h +2/-13
  • src/test/fuzz/coins_view.cpp +3/-11
  • src/test/fuzz/dbwrapper.cpp +1/-11
  • src/index/blockfilterindex.h +4/-2
  • src/validation.cpp +3/-3
  • src/index/base.h +4/-1
  • src/index/txospenderindex.cpp +3/-2
  • src/txdb.h +2/-3
  • test/functional/feature_init.py +5/-0
  • src/index/base.cpp +3/-1
  • src/index/coinstatsindex.h +3/-1
  • src/index/txindex.h +3/-1
  • src/index/txospenderindex.h +3/-1
  • src/node/chainstate.cpp +0/-4
  • test/functional/feature_utxo_abort_on_error.py +2/-2
  • src/kernel/chainstatemanager_opts.h +3/-0
  • src/node/chainstate.h +0/-2
  • src/test/fuzz/coinscache_sim.cpp +1/-1
  • src/txmempool.cpp +1/-1
  • src/txmempool.h +1/-1
  • test/functional/test_runner.py +1/-0

Card

The short text the ranking pass compares across PRs.

Centralizes LevelDB point-read and deserialization error handling in CDBWrapper, logging the error, invoking a fatal shutdown callback, and aborting rather than returning false or propagating exceptions. This eliminates the CCoinsViewErrorCatcher view layer, restricts HaveCoin() to the coins cache view, and marks coin view lookups noexcept. It prevents corrupted index reads from failing isolated RPCs while leaving a damaged node running, and stops corrupted entries from masquerading as missing keys.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-30T01:13, confidence high, input hash 922708395d6a48c8
thread read through 2026-10-29 (75 statements), head 846b0e83e1
labels: UTXO Db and Indexes

Categories

Indexes

P2 · bug fix

Prevents secondary indexes from silently treating corrupted database records as non-existent keys. As the author notes, previously 'a corrupt txindex point read can consequently fail one RPC while the node continues running' with damaged state.

Membership: Touches src/index/ base, txindex, blockfilterindex, coinstatsindex, and txospenderindex files to alter error handling on corrupt reads.

Factors: security/stability 2, bug 2, performance 0, user value 0, leverage 1

Validation

P2 · bug fix

Fixes a chainstate integrity hazard where LevelDB deserialization failures were reported as missing keys instead of fatal errors. It also removes the CCoinsViewErrorCatcher wrapper layer, eliminating unnecessary indirection across the coins view stack.

Membership: Has the 'UTXO Db and Indexes' label and modifies src/coins.*, src/txdb.*, src/dbwrapper.*, and src/node/chainstate.*.

Factors: security/stability 2, bug 2, performance 0, user value 0, leverage 1