#35728 rpc: Properly throw on internal I/O errors in GetTransaction
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #35728 rpc: Properly throw on internal I/O errors in GetTransaction | Ready | 1 (+5) | Strong | 80 + 86 tests | ||||||
|
|
|
| |||||||
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
Modify `node::GetTransaction` and `TxIndex` methods to throw `std::runtime_error` on block data I/O failures, deserialization errors, and index corruption instead of treating them as missing transactions. Update `getrawtransaction` to call `CheckBlockDataAvailability` on errors to provide explicit error messages for pruned or downloading blocks, and consolidate `ProcessPSBT` lookups to use `GetTransaction` directly.
Problem: When disk reads fail due to I/O errors, block file corruption, or index database corruption, `GetTransaction` and `TxIndex` return null or empty results. Callers like `getrawtransaction`, `gettxoutproof`, and `/rest/tx/` misinterpret this as a nonexistent transaction and report it as missing, misleading users and masking disk corruption.
Evidence of importance:
- Commit `fad44f18b8`: 'When a block hash is passed in and the block can't be read, the RPC reports "No such transaction found in the provided block", even though the block was never searched. Fix this by throwing an I/O error from GetTransaction when ReadBlock fails.'
- Commit `fa12509a47`: '...a -txindex corruption may also point to a larger system corruption and it seems better to notify the user properly by throwing an exception.'
Risk of a mistake: medium (rpc_api). Uncaught exceptions or unexpected error paths could crash the node process or disrupt RPC clients expecting standard JSON-RPC error codes.
Changed since the previous assessment: Added release notes in doc/release-notes-35728.md detailing the changed error behavior for getrawtransaction, gettxoutproof, utxoupdatepsbt, descriptorprocesspsbt, and /rest/tx/.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: clarify overlap with #34132 and reply to suggestions on WarnAndThrowErr and release notes 2026-11-01
Agreement
Strong
substantive support, no open objection (yuvicc, musaHaruna, yuvicc, ryanofsky, ryanofsky, ryanofsky)
Thread notes: Added l0rinc as a participant with three open items: a question on design overlap with PR #34132, a suggestion to simplify WarnAndThrowErr by accepting format arguments directly, and a question/suggestion regarding release note phrasing.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| yuvicc (contributor) | objection, correctness | In pruned mode, requesting a pruned block returns 'I/O error reading block data' instead of 'Block not available' | accepted · fix pushed · yuvicc confirmed the earlier concern has been addressed | changed | yes | 2026-07-16: 'In pruned mode when user calls `getrawtransaction <txid> <blockhash>` for the block that is pruned we return `"I/O error reading block data"` instead we should return `"Block not available"` which was' Status from: 2026-10-15: yuvicc: 'My earlier concern has been addressed.' |
| musaHaruna (contributor) | suggestion, safety | Wrap functional test assertions in a try...finally block so the renamed block file is always restored | answered · maflcko explained teardown deletes the temporary test directory anyway so restoring the file is unnecessary | answered | yes | 2026-07-16: 'would it be safer to wrap the assertions in a `try...finally` block so that the block file is always restored, even if an assertion or unexpected exception causes the test to exit early.' Status from: 2026-09-23: maflcko: 'I don’t think a finally block is needed here: if an assertion fails, the functional test’s temporary datadir is deleted ' |
| enirox001 (contributor) | suggestion, maintenance | Add a short release note explaining that some RPCs now report index or read errors instead of continuing with fallback lookups or reporting a missing transaction | accepted · fix pushed · maflcko added the release note and enirox001 confirmed | answered | yes | 2026-10-14: 'Would a short release note make sense for this, since some rpcs now report index or read errors where they previously continued with other lookups or reported a missing tx?' Status from: 2026-10-15: enirox001: 'Thanks for applying the changes. LGTM' |
| enirox001 (contributor) | question, approach | Whether throwing immediately in FindTx prevents callers from trying fallback lookups (such as UTXO set lookup in ProcessPSBT) that might succeed | accepted · maflcko clarified it is intentional to notify about index corruption early, and enirox001 agreed | answered | yes | 2026-10-14: 'I wonder if throwing immediately here could prevent callers from trying other lookups that might still succeed for example in `ProcessPSBT`' Status from: 2026-10-14: enirox001: 'Sounds good, letting the user know early on that the index is corrupted is a good idea' |
| musaHaruna (contributor) | suggestion, scope | Add CDBIterator::CheckStatus and call it in FindTx so LevelDB iterator errors throw instead of returning nullopt | accepted · maflcko noted this is a pre-existing issue that makes sense as a follow-up PR, and musaHaruna agreed to open one | answered | yes | 2026-10-16: 'Could we add this to the public methods of `CDBIterator` in `dbwrapper.h`?' Status from: 2026-10-16: musaHaruna: 'Yes I think a separate PR makes sense. I will open one as a follow-up to this.' |
| furszy (member) | suggestion, interface | Avoid mentioning internal function names like OpenBlockFile in the error message | accepted · furszy agreed to move forward in any direction rather than updating this PR | answered | yes | 2026-10-27: 'Mentioning "OpenBlockFile" in the error message doesn't seem the best for the user, it shares internals. May be better to change it to something like "Failed to open file for block hash %s".' Status from: 2026-10-27: furszy: 'np for me. Happy to move forward in any direction.' |
| ryanofsky (member) | question, scope | Whether to include the change skipping leftover txindex entries after an unclean shutdown in this PR or leave it for a follow-up | accepted · fix pushed · author decided to include it in this PR and implemented it | answered | yes | 2026-10-29: 'would you want to include the change suggested in the comment about leftover txindex entries after an unclean shutdown?' Status from: 2026-10-30: ryanofsky: 'Thanks, the new version looks good, and the other ways to reach these paths are a good point.' |
| ryanofsky (member) | suggestion, safety | Call sync_txindex before renaming blk00000.dat in rpc_rawtransaction.py to avoid intermittent test failures | accepted · author pointed out this is addressed in PR 36618, which ryanofsky accepted | answered | yes | 2026-10-29: 'The new test renames `blk00000.dat` before waiting for the txindex to finish syncing, and if the sync reads a block while the file is missing, the node shuts down. Calling `sync_txindex(self, node)` b' Status from: 2026-10-30: ryanofsky: 'Makes sense, thanks. Waiting for index sync whenever a node starts covers this rename too.' |
| ryanofsky (member) | suggestion, correctness | Catch FindTx errors and fall through to block read when a block hash is given in getrawtransaction | accepted · author explained reporting corruption is intentional, and ryanofsky agreed | answered | yes | 2026-10-29: 'Catching the `FindTx` error when a block hash is given and falling through to the block read would avoid this.' Status from: 2026-10-30: ryanofsky: 'Agree. Now that entries left behind by an unclean shutdown are skipped, an error from the txindex here means real corrup' |
| ryanofsky (member) | suggestion, maintenance | Clarify release note wording so internal error is not confused with RPC_INTERNAL_ERROR (-32603) | accepted · fix pushed · author explained why not to document the bug; ryanofsky agreed and author removed unclean wording | answered | yes | 2026-10-29: 'the release note's 'internal error' could be read as `RPC_INTERNAL_ERROR` (-32603), while these errors return `RPC_MISC_ERROR` (-1).' Status from: 2026-10-30: ryanofsky: 'Makes sense, agree it's better not to document a code we think is wrong. Thanks for removing "unclean".' |
| ryanofsky (member) | suggestion, correctness | Close the pruning race in getrawtransaction by re-checking block data availability if GetTransaction throws | accepted · fix pushed · author pushed a fix handling the race, which ryanofsky approved | answered | yes | 2026-10-29: 'the race this comment describes could be closed by checking again after the read fails' Status from: 2026-10-30: ryanofsky: 'Thanks! Agree, it should get simpler once `ReadBlock` reports why it failed.' |
| ryanofsky (member) | suggestion, approach | Skip txindex entries left behind by an unclean shutdown instead of throwing, so lookups do not permanently fail until the index is rebuilt | accepted · fix pushed · author implemented skipping leftover entries and ryanofsky accepted | answered | yes | 2026-10-29: 'skip index entries left behind by an unclean shutdown instead of throwing here and in the `BLOCK_HAVE_DATA` check below' Status from: 2026-10-30: ryanofsky: 'Thanks, the new version looks good, and the other ways to reach these paths are a good point. Fine without the unit test' |
| l0rinc (member) | question, approach | Clarify how database-read failures should be handled given overlap with #34132, which makes undecodable entries fatal in CDBWrapper and removes TryRead | open | answered | no | 2026-11-01: 'I’d like to clarify how we want these database-read failures handled so the two changes fit together.' |
| l0rinc (member) | suggestion, maintenance | Allow WarnAndThrowErr to accept the format string and arguments directly to avoid repetitive strprintf calls | open | answered | no | 2026-11-01: 'We could probably let `WarnAndThrowErr` take the format string and its arguments directly to get rid of the repeated `strprintf` wrappers (while keeping compile-time checks).' |
| l0rinc (member) | suggestion, maintenance | Reword the release note to describe block files failing to be read rather than block corruption being detected, since the Merkle root is not checked | open | answered | no | 2026-11-01: 'Is it accurate to say block corruption is detected here? As far as I can tell Merkle root isn't checked, so wouldn’t it be simpler to say the block files couldn’t be read?' |
Support:
- yuvicc (contributor) (Concept ACK, 2026-07-16) [read: TxIndex::FindTx, GetTransaction]: Distinguishing “transaction not found” from internal read failures in TxIndex::FindTx/GetTransaction makes sense.
- musaHaruna (contributor) (Concept ACK, 2026-07-16) [read]: separating expected outcomes ("transaction not found") from unexpected failures (I/O errors)
- enirox001 (contributor) (Concept ACK, 2026-10-14): Changes look good to me [not substantive]
- enirox001 (contributor) (ACK, 2026-10-15): Thanks for applying the changes. LGTM [not substantive]
- yuvicc (contributor) (ACK, 2026-10-15): My earlier concern has been addressed.
- musaHaruna (contributor) (Tested ACK, 2026-10-16) [tested]: Tested ACK fa87067 [not substantive]
- furszy (member) (Concept ACK, 2026-10-18): thanks for the ping @maflcko. Currently in vacation but will check it next week. [not substantive]
- sedited (member) (ACK, 2026-10-19): ACK fa87067c397814f79566f8e1b4f407745e3e1f8c [not substantive]
- furszy (member) (ACK, 2026-10-27) [read: OpenBlockFile]: It would be good to improve the "OpenBlockFile" mention in the err msg (see comment below). Happy to quickly re-ack if you do it. [not substantive]
- ryanofsky (member) (ACK, 2026-10-29) [read]: Good change: a confirmed transaction reported as missing is the wrong answer, and each path that turned an I/O or index error into "not found" now throws instead.
- ryanofsky (member) (ACK, 2026-10-30) [read: FindTx, REST endpoint]: FindTx skips leftover entries for blocks outside the active chain instead of throwing, the REST endpoint is tested, and the error message and two commit messages are clearer.
- ryanofsky (member) (ACK, 2026-10-30) [read: getrawtransaction, release note]: getrawtransaction possible race is avoided, and release note is updated to match latest state.
Participants:
- musaHaruna (contributor, support, 5 statements 2026-07-16 to 2026-10-16): Tested ACK; previously Concept ACK, suggested wrapping functional test assertions in try...finally, and suggested checking LevelDB iterator errors in FindTx which was agreed for a follow-up PR
- yuvicc (contributor, objection, 3 statements 2026-07-16 to 2026-10-15): Concept ACK, objected that pruned block queries returned an I/O error instead of 'Block not available', later confirmed addressed and ACKed
- enirox001 (contributor, support, 6 statements 2026-10-13 to 2026-10-15): Concept ACK, suggested adding a release note which was added, and ACKed
- furszy (member, support, 6 statements 2026-10-18 to 2026-10-27): ACK; previously Concept ACK, suggested avoiding internal function names like OpenBlockFile in error messages which was deferred to follow-ups
- sedited (member, support, 1 statements 2026-10-19 to 2026-10-19): ACK
- ryanofsky (member, support, 17 statements 2026-10-29 to 2026-10-30): ACK; suggested skipping leftover txindex entries after an unclean shutdown, closing pruning race, and clarifying release note, all of which were addressed
- l0rinc (member, question, 3 statements 2026-11-01 to 2026-11-01): asked to clarify overlap with #34132 regarding database-read failures, and suggested formatting and release note improvements
Files
86 lines under test/bench/ci.
- test/functional/feature_io_errors.py +63/-0
- src/index/txindex.cpp +41/-21
- src/rpc/rawtransaction.cpp +12/-17
- test/functional/rpc_rawtransaction.py +22/-0
- doc/release-notes-35728.md +17/-0
- src/node/transaction.cpp +10/-6
- test/functional/test_runner.py +1/-0
Card
The short text the ranking pass compares across PRs.
TxIndex and node::GetTransaction now throw std::runtime_error on disk I/O, deserialization, and index corruption failures rather than returning null. Currently, disk read errors or txindex corruption lead RPC and REST callers to report confirmed transactions as missing, concealing disk failure or corruption. This change causes endpoints such as getrawtransaction, gettxoutproof, PSBT processors, and /rest/tx/ to return internal errors instead of false 'not found' responses, while distinguishing pruned or downloading blocks with CheckBlockDataAvailability.
Data
thread read through 2026-11-01 (67 statements), head faa532fd36
labels: RPC/REST/ZMQ
Categories
Indexes
P2 · bug fix
TxIndex previously returned empty results on database read failures and corruption, causing callers to mistake index errors for missing transactions. Raising an explicit error prevents silent false negatives and ensures index and system corruption are surfaced immediately rather than masked.
Membership: Modifies TxIndex methods in src/index/txindex.cpp (+41/-21) to throw on block data I/O failures and database corruption.
Factors: security/stability 1, bug 2, performance 0, user value 2, leverage 0
RPC / REST / ZMQ
P2 · bug fix
Calls to getrawtransaction and REST endpoints misreported disk read failures as missing transactions ('No such transaction found in the provided block'). Differentiating internal read and corruption errors from absent transactions stops callers from receiving misleading status codes on hardware or block file errors.
Membership: Has RPC/REST/ZMQ label and modifies getrawtransaction error handling in src/rpc/rawtransaction.cpp and node::GetTransaction.
Factors: security/stability 1, bug 2, performance 0, user value 2, leverage 0