rpc: Properly throw on internal I/O errors in GetTransaction #35728

maflcko commented at 12:09 PM on July 15, 2026: member

Currently the GetTransaction return value is a nullable transaction pointer, where nullptr represents two different outcomes: I/O error and missing tx.

Reporting a confirmed transaction as missing is wrong, so properly return an error instead.

DrahtBot renamed this:
~~rpc: Properly throw on internal I/O errors in GetTransaction~~
rpc: Properly throw on internal I/O errors in GetTransaction
on Jul 15, 2026

DrahtBot added the label RPC/REST/ZMQ on Jul 15, 2026

DrahtBot commented at 12:09 PM on July 15, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35728
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35728.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35728#author-ralph

Reviews

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

Type	Reviewers
Stale ACK	enirox001, yuvicc, musaHaruna, sedited, furszy, ryanofsky

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:

#36597 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36597.svg"> (wallet: return broadcast errors from CommitTransaction by pseudoramdom)
#36566 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36566.svg"> (private broadcast: wait synchronously for mempool acceptance by andrewtoth)
#36002 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36002.svg"> (txindex: allow running in pruned mode by andrewtoth)
#31252 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/31252.svg"> (rpc: print P2WSH and P2SH redem Script in getrawtransaction and getblock by polespinasa)

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 internal implementation names in log messages:

src/index/txindex.cpp: LogWarning("%s", err) → Remove the GetName() prefix from the messages passed as err; it emits the internal index name (txindex) in the logs.

2026-10-30 08:58:08

maflcko force-pushed on Jul 15, 2026

DrahtBot added the label CI failed on Jul 15, 2026

DrahtBot commented at 12:15 PM on July 15, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/29414186507/job/87348122506 LLM reason (✨ experimental): CI failed due to a Python lint error from ruff (F401 unused import in test/functional/feature_io_errors.py).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

maflcko force-pushed on Jul 15, 2026

DrahtBot removed the label CI failed on Jul 15, 2026

yuvicc commented at 6:02 AM on July 16, 2026: contributor

Concept ACK Distinguishing “transaction not found” from internal read failures in TxIndex::FindTx/GetTransaction makes sense.

in src/rpc/rawtransaction.cpp:317 in 222207dcc1

yuvicc commented at 7:04 AM on July 16, 2026:

Do we ever reach here? 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 done previously?

functional test to check this behavior

    def test_block_without_data_is_not_io_error(self):
        node = self.nodes[0]
        txid = node.getblock(node.getblockhash(1))["tx"][0]
        tip = node.getbestblockhash()
        block = create_block(
            int(tip, 16),
            create_coinbase(node.getblockcount() + 1),
            ntime=node.getblock(tip)["time"] + 1,
        )
        block.solve()
        node.submitheader(block.serialize()[:80].hex())

        assert_raises_rpc_error(-1, "Block not available", node.getrawtransaction, txid, 1, block.hash_hex)

</details>

maflcko commented at 4:30 PM on September 23, 2026:

Hmm, I think the "I/O error reading block data" is correct, even when pruning.

The correct way to handle this is to call CheckBlockDataAvailability early, and then throw the "I/O error reading block data" in GetTransaction.

Waiting for the txindex and the blockindex to "fall through" to return a nullptr makes it impossible to uncover IO errors and makes it impossible to distinguish IO errors with normal missing data.

The goal of this pull is to fix exactly that.

(Edit: I've pushed your test draft and my fix)

musaHaruna commented at 11:51 AM on July 16, 2026: contributor

Concept ACK on separating expected outcomes ("transaction not found") from unexpected failures (I/O errors).

in test/functional/feature_io_errors.py:47 in fac83ef8e9 outdated

  42 | +        msg = "I/O error reading block data"
  43 | +        assert_raises_rpc_error(-32603, msg, node.getrawtransaction, "a" * 64, blockhash=block)
  44 | +        msg = "Can't read block from disk"
  45 | +        assert_raises_rpc_error(-32603, msg, node.gettxoutproof, [tx2["txid"]])
  46 | +
  47 | +        blk_dat_moved.rename(blk_dat)

musaHaruna commented at 11:59 AM on July 16, 2026:

        # Simulate a block file I/O failure by temporarily renaming the block file.
        blk_dat.rename(blk_dat_moved)

        try:
            msg = "I/O error while opening block file via txindex"
            assert_raises_rpc_error(-32603, msg, node.gettxoutproof, [txid])
            assert_raises_rpc_error(-32603, msg, node.getrawtransaction, txid)
            if self.is_wallet_compiled():
                assert_raises_rpc_error(-32603, msg, node.utxoupdatepsbt, psbt)
            msg = "I/O error reading block data"
            assert_raises_rpc_error(-32603, msg, node.getrawtransaction, "a" * 64, blockhash=block)
            msg = "Can't read block from disk"
            assert_raises_rpc_error(-32603, msg, node.gettxoutproof, [tx2["txid"]])
        finally:
            blk_dat_moved.rename(blk_dat)

Some small suggestions:

Will it be worth it to add a brief comment before renaming blk00000.dat to clarify that the rename is intentionally simulating a block file I/O failure?

Since the test temporarily renames blk00000.dat to simulate an I/O failure, 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.

maflcko commented at 12:07 PM on July 16, 2026:

Since the test temporarily renames blk00000.dat to simulate an I/O failure, 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.

Why would it be safer to rename a file that will be deleted a tick later?

musaHaruna commented at 1:10 PM on July 16, 2026:

Apologies not sure if I understand the question, but what I mean by wrapping in try...finally is:

blk_dat.rename(blk_dat_moved)

msg = "I/O error while opening block file via txindex"
assert_raises_rpc_error(-32603, msg, node.getrawtransaction, txid)

# Suppose the assertion above fails.
# Execution never reaches this line.
blk_dat_moved.rename(blk_dat)

If node.getrawtransaction(txid) does not return the expected RPC error (for example, due to a regression). Now the code is left with blk00000.dat.moved.

So my thought is with try...finally even if the assertion fails, the finally block executes first, and the node is restored to its original state.

So my initial comment is more of a question; would it be safer if we wrap the assertations in a try...finally.

maflcko commented at 4:35 PM on September 23, 2026:

Apologies not sure if I understand the question

It was just a rhetorical question with the answer: "I don’t think a finally block is needed here: if an assertion fails, the functional test’s temporary datadir is deleted during teardown, so restoring the renamed file provides no practical benefit."

maflcko marked this as a draft on Aug 7, 2026

DrahtBot added the label Needs rebase on Aug 15, 2026

maflcko marked this as ready for review on Sep 23, 2026

maflcko force-pushed on Sep 23, 2026

DrahtBot added the label CI failed on Sep 23, 2026

maflcko removed the label CI failed on Sep 23, 2026

DrahtBot removed the label Needs rebase on Sep 23, 2026

DrahtBot added the label Needs rebase on Sep 26, 2026

maflcko force-pushed on Oct 6, 2026

test: Add test for getrawtransaction block read errors fa5ad60c32

maflcko force-pushed on Oct 12, 2026

DrahtBot added the label CI failed on Oct 12, 2026

DrahtBot commented at 10:30 AM on October 12, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/38228957902/job/114743378818 LLM reason (✨ experimental): CI failed because IWYU removed the unused <atomic> include from src/index/txindex.cpp, leaving an uncommitted diff that the check rejects.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

maflcko force-pushed on Oct 12, 2026

DrahtBot removed the label Needs rebase on Oct 12, 2026

DrahtBot removed the label CI failed on Oct 12, 2026

in src/node/transaction.cpp:165 in fa5b2f13da outdated

 166 | -                if (tx->GetHash() == hash) {
 167 | -                    hashBlock = block_index->GetBlockHash();
 168 | -                    return tx;
 169 | -                }
 170 | +        if (!blockman.ReadBlock(block, *block_index)) {
 171 | +            throw std::runtime_error{strprintf("I/O error reading block data for block %s", block_index->GetBlockHash().ToString())};

enirox001 commented at 11:42 PM on October 13, 2026:

In commit https://github.com/bitcoin/bitcoin/pull/35728/changes/fa5b2f13da03d295ab624190f0d393b13da5b929: rpc: Return proper error messages from getrawtransaction

nit: the header file should be updated to mention that this can throw now

index bf92869d3e..e7ee5ee1a4 100644
--- a/src/node/transaction.h
+++ b/src/node/transaction.h
@@ -71,6 +71,7 @@ inline constexpr CAmount DEFAULT_MAX_BURN_AMOUNT{0};
  * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in]  blockman        Used to access and read blocks from disk
  * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[out] hashBlock       The block hash, if the tx was found via -txindex or block_index
  * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/)                    The tx if found, otherwise nullptr
+ * [@throws](/github-metadata-backup-bitcoin-bitcoin/contributor/throws/) a std::runtime_error if block data cannot be read.
  */
 CTransactionRef GetTransaction(const CBlockIndex* block_index, const CTxMemPool* mempool, const Txid& hash, const BlockManager& blockman, uint256& hashBlock);
 } // namespace node

maflcko commented at 1:51 PM on October 14, 2026:

This is already true on current master: It calls m_db->Read(...), which can throw dbwrapper_error, which is also a runtime_error. So my preference would be to leave this as-is for now. If you really want, I can add a unrelated first commit to document this, but I think it is fine.

furszy commented at 2:08 PM on October 27, 2026:

Not for this PR, but instead of having to hardcode the error msg at this layer again, we could make ReadBlock return a Result<CBlock, ErrCase> with ErrCase containing an enum for the error type and a error msg string.

maflcko commented at 4:35 PM on October 27, 2026:

Yeah, seems fine. Personally I think it would be fine to also return std::optional<CBlock> and just log the error. After all, this should be relatively rare, so UX doesn't need to be too fine. But no strong opinion.

maflcko commented at 8:58 AM on October 30, 2026:

(resolving for now)

in src/index/txindex.cpp:199 in 3333a91dbf

 198 | -                continue;
 199 | +                WarnAndThrowErr(strprintf("%s: Block index entry %s not found for txid %s. Caused by data corruption or possibly an unclean shutdown?",
 200 | +                                          GetName(), candidate_block_hash.ToString(), tx_hash.ToString()));
 201 | +            }
 202 | +            if (!(block_index->nStatus & BLOCK_HAVE_DATA)) {
 203 | +                WarnAndThrowErr(strprintf("%s: Block index entry %s unexpectedly unavailable for txid %s. Caused by data corruption or possibly an unclean shutdown?",

enirox001 commented at 1:36 AM on October 14, 2026:

In commit https://github.com/bitcoin/bitcoin/pull/35728/changes/3333a91dbf1eac659cd7d027bc891a80241e8f66: rpc: Properly throw on internal FindTx IO errors in ProcessPSBT and GetTransaction

nit: Here I think what is missing is the block data and not the index

index d608e733f3..9ec5c2e857 100644
--- a/src/index/txindex.cpp
+++ b/src/index/txindex.cpp
@@ -196,7 +196,7 @@ std::optional<TxIndexResult> TxIndex::FindTx(const Txid& tx_hash) const
                                           GetName(), candidate_block_hash.ToString(), tx_hash.ToString()));
             }
             if (!(block_index->nStatus & BLOCK_HAVE_DATA)) {
-                WarnAndThrowErr(strprintf("%s: Block index entry %s unexpectedly unavailable for txid %s. Caused by data corruption or possibly an unclean shutdown?",
+                WarnAndThrowErr(strprintf("%s: Block data %s unexpectedly unavailable for txid %s. Caused by data corruption or possibly an unclean shutdown?",
                                           GetName(), candidate_block_hash.ToString(), tx_hash.ToString()));
             }
             const FlatFilePos tx_position{block_index->nFile, block_index->nDataPos + key.pos.tx_offset_in_block};

maflcko commented at 1:51 PM on October 14, 2026:

thx, done

in src/index/txindex.cpp:167 in 3333a91dbf outdated

 162 | +{
 163 | +    LogWarning("%s", err);
 164 | +    throw std::runtime_error{err};
 165 | +}
 166 | +
 167 |  std::optional<TxIndexResult> TxIndex::FindTx(const Txid& tx_hash) const

enirox001 commented at 1:44 AM on October 14, 2026:

In commit https://github.com/bitcoin/bitcoin/pull/35728/changes/3333a91dbf1eac659cd7d027bc891a80241e8f66#r4265873624: rpc: Properly throw on internal FindTx IO errors in ProcessPSBT and GetTransaction

nit: the header documentation for this method should be updated to mention it can now throw

index 23fb17d431..c72251273b 100644
--- a/src/index/txindex.h
+++ b/src/index/txindex.h
@@ -63,6 +63,7 @@ public:
     ///
     /// [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in]   tx_hash  The hash of the transaction to be returned.
     /// [@return](/github-metadata-backup-bitcoin-bitcoin/contributor/return/)  the transaction and containing block hash, or nullopt if it is not found
+    /// [@throws](/github-metadata-backup-bitcoin-bitcoin/contributor/throws/) a std::runtime_error If an I/O or transaction index error occurs.
     std::optional<TxIndexResult> FindTx(const Txid& tx_hash) const;
 };

maflcko commented at 1:51 PM on October 14, 2026:

This is already true on current master: It calls m_db->Read(...), which can throw dbwrapper_error, which is also a runtime_error. So my preference would be to leave this as-is for now. If you really want, I can add a unrelated first commit to document this, but I think it is fine.

enirox001 commented at 2:07 AM on October 14, 2026: contributor

Concept ACK

fa5ad60c32 introduced test_getrawtransaction_block_data_errors showing default errors when the block is not available and when the block cannot be read
fa5b2f13da getrawtransaction no longer checks the block availability inline, task delegated to CheckBlockDataAvailability, now I/O errors can be detected in GetTransaction and thrown
fa8a5b033b introduced feature_io_errors.py to test I/O errors, creates a psbt and makes the block file unavailable, showing default behavior when rpc commands try to read this file
3333a91dbf FindTx still returns nullopt for a missing tx, but on the I/O and index, errors are handled here, we now log the warning and throw instead of continuing (updated test to use the change)
faa4bb35ae In ProcessPSBT now using GetTransaction instead of a more verbose tx lookup

Changes look good to me, left a few nits, I do have a question though;

I wonder if throwing immediately here could prevent callers from trying other lookups that might still succeed for example in ProcessPSBT, we first call GetTransaction which checks the mempool and txindex, if the tx isnt found we use FindCoins to look in the utxo set

But if FindTx throws, we never reach that last check, even though the utxo set might still have the input information.

maflcko force-pushed on Oct 14, 2026

maflcko commented at 1:51 PM on October 14, 2026: member

I wonder if throwing immediately here could prevent callers from trying other lookups that might still succeed for example in ProcessPSBT, we first call GetTransaction which checks the mempool and txindex, if the tx isnt found we use FindCoins to look in the utxo set

But if FindTx throws, we never reach that last check, even though the utxo set might still have the input information.

Correct. But this is intentional: I'd say if someone is running with a corrupt index (or an index with stale data from an unclean shutdown, it would be good to notify them about it as early as possible. This way, they can either disable the index, or recreate it by deleting the index folder. Somehow adding extra fallback code means they'll still run into the issue, just later.

enirox001 commented at 2:56 PM on October 14, 2026: contributor

Correct. But this is intentional: I'd say if someone is running with a corrupt index (or an index with stale data from an unclean shutdown, it would be good to notify them about it as early as possible. This way, they can either disable the index, or recreate it by deleting the index folder. Somehow adding extra fallback code means they'll still run into the issue, just later.

Sounds good, letting the user know early on that the index is corrupted is a good idea, 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?

maflcko commented at 12:19 PM on October 15, 2026: member

thx, added release note

enirox001 commented at 3:10 PM on October 15, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/pull/35728/changes/fa87067c397814f79566f8e1b4f407745e3e1f8c

Thanks for applying the changes. LGTM

DrahtBot requested review from yuvicc on Oct 15, 2026

DrahtBot requested review from musaHaruna on Oct 15, 2026

yuvicc commented at 3:38 PM on October 15, 2026: contributor

lgmt! ACK fa87067c397814f79566f8e1b4f407745e3e1f8c

My earlier concern has been addressed.

musaHaruna commented at 10:48 AM on October 16, 2026: contributor

While testing this, I think I might have found another case where FindTx() returns nullopt instead of throwing.

I first confirmed that a transaction could be retrieved, then deliberately damaged part of the test’s temporary transaction-index database. LevelDB detected the corruption, but FindTx() still returned nullopt without throwing. Restoring the original data made the lookup work again.

It looks like the candidate scan checks whether the iterator has a valid entry, but does not check whether a database error occurred. That means a failed lookup can be treated as an ordinary missing transaction.

Could we add this to the public methods of CDBIterator in dbwrapper.h?

/** Throw dbwrapper_error if the iterator encountered a database error. */
void CheckStatus() const;

Then implement it in dbwrapper.cpp, using the existing error handler to throw when LevelDB reports an error:

void CDBIterator::CheckStatus() const
{
    HandleError(m_impl_iter->iter->status());
}

Finally, call it immediately after the candidate-collection loop in FindTx(), before the iterator goes out of scope:

candidates.emplace_back(
            tx_position,
            candidate_block_hash,
            key.pos.block_seq,
            m_chainstate->m_chain.Contains(*block_index));
    }

    it->CheckStatus();
}

This would let an ordinary missing transaction still return nullopt, while reporting a database error instead of silently treating it as missing.

DrahtBot requested review from musaHaruna on Oct 16, 2026

maflcko commented at 10:59 AM on October 16, 2026: member

While testing this, I think I might have found another case where FindTx() returns nullopt instead of throwing.

Yes, this seems plausible. I think Ralph LLM Review also says this. IIRC, I had a LLM say - [P2] src/index/txindex.cpp:187: iterator failures still become nullopt. A LevelDB iterator I/O error makes Valid() false, while malformed key data makes GetKey() false. Both terminate the loop and can still produce “transaction not found.” at one point.

However, the goal here is to primarily map the existing errors from a warning log to an actual real error. Happy to look into the dbwrapper itself later, but I think there are a bunch of other pulls open (or in the plans), so I don't want to create merge conflicts there.

Also, I think this is a pre-existing issue, which is true before and after this pull request, so a separate pull for this could make sense?

musaHaruna commented at 11:06 AM on October 16, 2026: contributor

Also, I think this is a pre-existing issue, which is true before and after this pull request, so a separate pull for this could make sense?

Yes I think a separate PR makes sense. I will open one as a follow-up to this.

Tested ACK fa87067

maflcko commented at 11:30 AM on October 16, 2026: member

cc @furszy I created this in reply to #35003, and to unblock the index-pessimisation from 35003: 35003 turns a correct rpc_error(-1, "filesystem error", node.getrawtransaction into a buggy rpc_error(-5, "No such mempool or blockchain transaction", node.getrawtransaction. So you may be qualified and interested to review/nack/ack this?

furszy commented at 3:32 PM on October 18, 2026: member

Concept ACK, thanks for the ping @maflcko. Currently in vacation but will check it next week.

sedited approved

sedited commented at 11:50 AM on October 19, 2026: contributor

ACK fa87067c397814f79566f8e1b4f407745e3e1f8c

DrahtBot requested review from furszy on Oct 19, 2026

in test/functional/feature_io_errors.py:38 in fa5c96a112

  36 | -        assert_raises_rpc_error(-5, "No such mempool or blockchain transaction", node.getrawtransaction, txid)
  37 | -        node.utxoupdatepsbt(psbt)  # no exception raised on IO error
  38 | +        msg = "OpenBlockFile failed"
  39 | +        assert_raises_rpc_error(-1, msg, node.gettxoutproof, [txid])
  40 | +        assert_raises_rpc_error(-1, msg, node.getrawtransaction, txid)
  41 | +        assert_raises_rpc_error(-1, msg, node.utxoupdatepsbt, psbt)

furszy commented at 2:18 PM on October 27, 2026:

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

maflcko commented at 3:15 PM on October 27, 2026:

I guess I agree, but this was taken from the prior debug log warning message.

This has 5 acks, so maybe a trivial (unrelated) commit can be added to #35003 , or some of the other follow-ups?

furszy commented at 3:23 PM on October 27, 2026:

np for me. Happy to move forward in any direction.

maflcko commented at 4:34 PM on October 27, 2026:

I pushed https://github.com/maflcko/DrahtBot/commit/e2ba727520de760409b743e2962ef6d2215a7a77, for the DrahtBot LLM which prints:

src/index/txindex.cpp: WarnAndThrowErr(strprintf("%s: OpenBlockFile failed for txid %s at %s", ...)) → Replace OpenBlockFile failed with plain-language wording such as “Unable to open block file”; it exposes an internal function name in the warning.

Should avoid this wording in the future, maybe.

furszy commented at 4:56 PM on October 27, 2026:

nice!

maflcko commented at 8:58 AM on October 30, 2026:

thx, reworded debug log msg

furszy commented at 2:22 PM on October 27, 2026: member

ACK fa87067c397814f79566f8e1b4f407745e3e1f8c

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.

maflcko commented at 1:52 PM on October 29, 2026: member

rfm with 5 acks, or should I address the log msg nit, or does it need more review?

in src/index/txindex.cpp:161 in fa87067c39

 157 | @@ -156,6 +158,12 @@ bool TxIndex::CustomAppend(const interfaces::BlockInfo& block)
 158 |  
 159 |  BaseIndex::DB& TxIndex::GetDB() const { return *m_db; }
 160 |  
 161 | +[[noreturn]] void WarnAndThrowErr(const std::string& err)

ryanofsky commented at 4:40 PM on October 29, 2026:

In commit "rpc: Properly throw on internal FindTx IO errors in ProcessPSBT and GetTransaction" (fa5c96a112c2d11e32278838cb43bae243f6e951)

Nit for a follow-up: this could be static (or live in the anonymous namespace above), since it is only used in this file and has no declaration in the header.

Update: the Ralph review also flags this.

maflcko commented at 8:58 AM on October 30, 2026:

thx, added static

in doc/release-notes-35728.md:17 in fa87067c39 outdated

  12 | +Updated REST APIs
  13 | +-----------------
  14 | +
  15 | +- The `/rest/tx/<TX-HASH>` endpoint now returns an internal error when block
  16 | +  files are corrupt or the transaction index is corrupt or in an unclean state.
  17 | +  Previously, it reported the transaction as not found.

ryanofsky commented at 4:40 PM on October 29, 2026:

In commit "Add doc/release-notes-35728.md" (fa87067c397814f79566f8e1b4f407745e3e1f8c)

Note for reviewers: rest_tx doesn't catch the new exception itself, but httpserver.cpp wraps every handler in a catch (const std::exception&) that replies HTTP 500 with the message, so this REST behavior is right. It isn't covered by feature_io_errors.py, which only exercises RPC; a /rest/tx/<txid> check there would be a small follow-up.

Update: the Ralph review also flags this.

maflcko commented at 8:58 AM on October 30, 2026:

thx, added REST test

in src/rpc/rawtransaction.cpp:316 in fa87067c39 outdated

 311 | @@ -319,6 +312,8 @@ static RPCMethod getrawtransaction()
 312 |          if (!blockindex) {
 313 |              throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found");
 314 |          }
 315 | +        CheckBlockDataAvailability(chainman.m_blockman, *blockindex, /*check_for_undo=*/false);
 316 | +        // The data could be pruned after this check, causing GetTransaction to throw an I/O error.

ryanofsky commented at 4:54 PM on October 29, 2026:

In commit "rpc: Return proper error messages from getrawtransaction" (fa5b2f13da03d295ab624190f0d393b13da5b929)

Not blocking, but the race this comment describes could be closed by checking again after the read fails. The old order had no race: it checked BLOCK_HAVE_DATA only after the read had failed, so a block pruned in between was reported as "Block not available". With the check moved before the read, the same race now produces "I/O error reading block data". Keeping this up-front check for the better messages and re-running it when GetTransaction throws would report pruned data correctly in both cases. For example:

CTransactionRef tx;
try {
    tx = GetTransaction(blockindex, node.mempool.get(), txid, chainman.m_blockman, hash_block);
} catch (const std::runtime_error&) {
    if (blockindex) WITH_LOCK(::cs_main, CheckBlockDataAvailability(chainman.m_blockman, *blockindex, /*check_for_undo=*/false));
    throw;
}

Alternatively, GetTransaction could check BLOCK_HAVE_DATA itself after a failed ReadBlock and only throw the I/O error if the flag is still set. Either would fit in a follow-up.

Update: the Ralph review also flags this.

maflcko commented at 8:58 AM on October 30, 2026:

Hmm, I think this is true for all places that call CheckBlockDataAvailability, so I think I'll leave as-is for now. Maybe someone can fix this later for all places? Maybe like furszy suggested in #35728 (review) to return the error message (and possibly type) from the blockstorage module?

in src/node/transaction.cpp:166 in fa87067c39 outdated

 167 | -                    hashBlock = block_index->GetBlockHash();
 168 | -                    return tx;
 169 | -                }
 170 | +        if (!blockman.ReadBlock(block, *block_index)) {
 171 | +            throw std::runtime_error{strprintf("I/O error reading block data for block %s", block_index->GetBlockHash().ToString())};
 172 | +        }

ryanofsky commented at 4:54 PM on October 29, 2026:

In commit "rpc: Return proper error messages from getrawtransaction" (fa5b2f13da03d295ab624190f0d393b13da5b929)

Nit, only if you push again: the commit message opens by describing the previous error messages and saying it is fixing them, but those messages are actually correct and make sense. The actual bug is described later in the second paragraph: a block that fails to read is reported as "No such transaction found in the provided block". Suggested rewrite:

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.

Because the not-found branch no longer runs when the read fails, check
block availability up front with CheckBlockDataAvailability. This also
gives more specific errors for pruned and not yet downloaded blocks
than the previous "Block not available".

maflcko commented at 8:58 AM on October 30, 2026:

thx, used your commit msg

in src/index/txindex.cpp:201 in fa87067c39

 200 | +                                          GetName(), candidate_block_hash.ToString(), tx_hash.ToString()));
 201 | +            }
 202 | +            if (!(block_index->nStatus & BLOCK_HAVE_DATA)) {
 203 | +                WarnAndThrowErr(strprintf("%s: Block data %s unexpectedly unavailable for txid %s. Caused by data corruption or possibly an unclean shutdown?",
 204 | +                                          GetName(), candidate_block_hash.ToString(), tx_hash.ToString()));
 205 |              }

ryanofsky commented at 5:12 PM on October 29, 2026:

In commit "rpc: Properly throw on internal FindTx IO errors in ProcessPSBT and GetTransaction" (fa5c96a112c2d11e32278838cb43bae243f6e951)

Suggestion for a follow-up: skip index entries left behind by an unclean shutdown instead of throwing here and in the BLOCK_HAVE_DATA check below. Otherwise a crash at the wrong moment leaves some transactions permanently failing to look up with this error, even though nothing is corrupt, and the only way out is rebuilding the whole txindex.

The leftovers happen because WriteTxs writes entries as soon as a block is connected, while the block index is only flushed every 50–70 minutes. After a crash, the txindex can have entries for a block the node no longer knows about, or knows only as a header. If that block is connected again, everything lines up, but if it went stale in the meantime, its entries stay forever.

Such a block can't be in the active chain, so skipping its entries gives the same result as if it had never been indexed, which is what the code did before this PR. Missing data for a block that is in the active chain does mean corruption, and the change below still throws for that:

             LOCK(cs_main);
             const CBlockIndex* block_index{m_chainstate->m_blockman.LookupBlockIndex(candidate_block_hash)};
-            if (!block_index) {
-                WarnAndThrowErr(strprintf("%s: Block index entry %s not found for txid %s. Caused by data corruption or possibly an unclean shutdown?",
-                                          GetName(), candidate_block_hash.ToString(), tx_hash.ToString()));
-            }
+            // Entries are written as soon as a block is connected, before the
+            // block index is flushed, so after an unclean shutdown they can
+            // refer to a block whose index entry or data was never written.
+            // If the block was not connected again, it is not in the active
+            // chain, and skipping it gives the same result as if it had never
+            // been indexed. Only missing data for an active-chain block
+            // indicates corruption.
+            if (!block_index) continue;
+            const bool in_active_chain{m_chainstate->m_chain.Contains(*block_index)};
             if (!(block_index->nStatus & BLOCK_HAVE_DATA)) {
-                WarnAndThrowErr(strprintf("%s: Block data %s unexpectedly unavailable for txid %s. Caused by data corruption or possibly an unclean shutdown?",
+                if (!in_active_chain) continue;
+                WarnAndThrowErr(strprintf("%s: Block data %s unexpectedly unavailable for txid %s. Caused by data corruption?",
                                           GetName(), candidate_block_hash.ToString(), tx_hash.ToString()));
             }
             const FlatFilePos tx_position{block_index->nFile, block_index->nDataPos + key.pos.tx_offset_in_block};
-            candidates.emplace_back(tx_position, candidate_block_hash, key.pos.block_seq, m_chainstate->m_chain.Contains(*block_index));
+            candidates.emplace_back(tx_position, candidate_block_hash, key.pos.block_seq, in_active_chain);

diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp
index 0eec1379b32..68cd28660f9 100644
--- a/src/test/txindex_tests.cpp
+++ b/src/test/txindex_tests.cpp
@@ -21,6 +21,7 @@
 #include <script/script.h>
 #include <streams.h>
 #include <sync.h>
+#include <test/util/common.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
 #include <util/byte_units.h>
@@ -352,4 +353,45 @@ BOOST_FIXTURE_TEST_CASE(txindex_reorg_keeps_stale_entries, TestChain100Setup)
     StopIndex(txindex, m_node);
 }

+BOOST_FIXTURE_TEST_CASE(txindex_unclean_shutdown_leftovers, TestChain100Setup)
+{
+    TxIndex txindex(interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB, /*f_memory=*/true);
+    BOOST_REQUIRE(txindex.Init());
+    txindex.Sync();
+
+    CDBWrapper& db{TxIndexTest::GetDB(txindex)};
+    const Txid txid{m_coinbase_txns.front()->GetHash()};
+    const auto prefix{txindex::CreateKeyPrefix(ReadHasher(db), txid)};
+    const auto bucket{BucketPositions(db, prefix)};
+    BOOST_REQUIRE_EQUAL(bucket.size(), 1U);
+    const uint256 active_block_hash{LookupTx(txindex, txid)};
+    uint32_t next_seq{0};
+    BOOST_REQUIRE(db.Read(txindex::DB_NEXT_BLOCK_SEQ, next_seq));
+
+    // Simulate entries written for a block that was connected, but whose block
+    // index entry was not flushed before an unclean shutdown.
+    db.Write(txindex::BlockSeqKey{next_seq}, m_rng.rand256());
+    db.Write(txindex::DBKey{prefix, {next_seq, bucket.front().tx_offset_in_block}}, txindex::EMPTY_VALUE);
+    BOOST_CHECK(LookupTx(txindex, txid) == active_block_hash);
+
+    // Simulate entries written for a block whose header was flushed but whose
+    // data was not, and which never became part of the active chain again.
+    const CBlock header_only{CreateBlock({}, CScript() << OP_TRUE)};
+    {
+        BlockValidationState state;
+        BOOST_REQUIRE(m_node.chainman->ProcessNewBlockHeaders({{static_cast<const CBlockHeader&>(header_only)}}, /*min_pow_checked=*/true, state));
+    }
+    db.Write(txindex::BlockSeqKey{next_seq + 1}, header_only.GetHash());
+    db.Write(txindex::DBKey{prefix, {next_seq + 1, bucket.front().tx_offset_in_block}}, txindex::EMPTY_VALUE);
+    BOOST_CHECK(LookupTx(txindex, txid) == active_block_hash);
+
+    // Missing data for a block in the active chain is still an error.
+    CBlockIndex* active_index{WITH_LOCK(cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(active_block_hash))};
+    WITH_LOCK(cs_main, active_index->nStatus &= ~BLOCK_HAVE_DATA);
+    BOOST_CHECK_EXCEPTION(txindex.FindTx(txid), std::runtime_error, HasReason("unexpectedly unavailable"));
+    WITH_LOCK(cs_main, active_index->nStatus |= BLOCK_HAVE_DATA);
+
+    StopIndex(txindex, m_node);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

</details>

Update: the Ralph review also flags this.

maflcko commented at 8:58 AM on October 30, 2026:

and the only way out is rebuilding the whole txindex.

Right. When I wrote this back in July, this was intentional, because I assumed that unclean shutdowns are rare enough to just let the user quickly create a fresh txindex. However, I think the picture has changed in the meantime:

Ralph LLM flagged this, so other bots will likely flag and fix this when this is merged. Also, it could make Fuzzamoto/bedrock testing harder, because it can surface these rare errors.
There are plans to support pruning while having indexes, so quickly re-creating an index may actually be tedious.

I didn't take your comment, because the code paths are also reachable when doing something fancy such as:

Run with txindex and reorg out a block
Drop block folder and sync again with txindex disabled
Restart with txindex and submit the stale block header

There is probably no need to enumerate all of them in the code. Modern LLMs should be capable to enumerate them for anyone who needs them.

I think a unit test is not needed, so I'll skip it for now.

in src/rpc/rawtransaction.cpp:179 in fa87067c39 outdated

 184 | -        // If we still don't have it look in the mempool
 185 | -        if (!tx) {
 186 | -            tx = node.mempool->get(psbt_input.prev_txid);
 187 | -        }
 188 | +        uint256 block_hash;
 189 | +        CTransactionRef tx{GetTransaction(/*block_index=*/nullptr, node.mempool.get(), psbt_input.prev_txid, node.chainman->m_blockman, block_hash)};

ryanofsky commented at 5:42 PM on October 29, 2026:

In commit "refactor: Use GetTransaction in ProcessPSBT" (fa6a70716a5256476c469a08c4b460d5808396d5)

Nit, only if you push again: it would help to say why this commit is here, since nothing else in the PR depends on it. As far as I can tell it's an independent cleanup. ProcessPSBT repeated the txindex and mempool lookups that GetTransaction already does, and it picks up the new errors in the previous commit by calling FindTx directly. It is also not a strict refactor, though the difference only shows up on errors: GetTransaction checks the mempool first, so a prevout in the mempool is now found without reading the txindex, and an index error no longer fails the RPC for it. Maybe something like:

ProcessPSBT repeats the txindex and mempool lookups in GetTransaction.
Call GetTransaction instead, so the lookup and its error handling are
in one place.

GetTransaction checks the mempool before the txindex, so a prevout in
the mempool is now found without reading the index.

maflcko commented at 8:58 AM on October 30, 2026:

thx, used your commit msg

ryanofsky approved

ryanofsky commented at 6:18 PM on October 29, 2026: contributor

Code review ACK fa87067c397814f79566f8e1b4f407745e3e1f8c. 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.

Agree this looks ready for merge, since the remaining suggestions are agreed to be follow-ups: the "OpenBlockFile" wording #35728 (review) and checking the LevelDB iterator status after the FindTx scan #35728#pullrequestreview-5546709537. The latter fixes similar bugs that happen one level lower at the database layer.

One question before merging: would you want to include the change suggested in the comment about leftover txindex entries after an unclean shutdown? If a node crashes shortly after connecting a block that later goes stale, the txindex keeps leftover entries for that block. Before this PR, getrawtransaction, gettxoutproof and utxoupdatepsbt skipped those entries and returned the correct result. With this PR, calls that run into them fail with an internal error, and keep failing until the txindex is rebuilt.

ryanofsky commented at 7:13 PM on October 29, 2026: contributor

Notes on the Ralph review which mostly covers the same ground as mine: (unclean shutdown leftovers, pruning race, REST test, WarnAndThrowErr linkage), and has two findings that are new to me:

rpc_rawtransaction.py could fail intermittently. 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) before the rename avoids this.
A bad index row can fail getrawtransaction txid blockhash. With -txindex, the RPC looks in the txindex before reading the requested block, so a bad index row for a different block now fails the call even when the requested block is readable and contains the transaction. Catching the FindTx error when a block hash is given and falling through to the block read would avoid this.

It also points out that the release note's "internal error" could be read as RPC_INTERNAL_ERROR (-32603), while these errors return RPC_MISC_ERROR (-1).

rpc: Return proper error messages from getrawtransaction

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.

Because the not-found branch no longer runs when the read fails, check
block availability up front with CheckBlockDataAvailability. This also
gives more specific errors for pruned and not yet downloaded blocks
than the previous "Block not available".

faed3d2b3f

test: Add test to check RPCs throw on IO errors fab57c7d90

rpc: Properly throw on internal FindTx IO errors in ProcessPSBT and GetTransaction

Currently, the FindTx return value is an optional, where nullopt
represents two different outcomes: I/O error and missing tx.

One may argue that this is fine in ProcessPSBT, because a -txindex
lookup error is rare, and an error where the fallbacks to lookup the tx
are also failing is even rarer. However, a -txindex corruption may also
point to a larger system corruption and it seems better to notify the
user properly by throwing an exception.

If I/O errors should be ignored in the future, it would be trivial to
catch them here. However, that decision should be up to the
caller, and not enforced by the low-level function.

Do the same for GetTransaction:
Currently the GetTransaction return value is a nullable transaction
pointer, where nullptr represents two different outcomes: I/O error and
missing tx.

Reporting a confirmed transaction as missing is wrong, so properly
return an error instead.

fa48965270

refactor: Use GetTransaction in ProcessPSBT

ProcessPSBT repeats the txindex and mempool lookups in GetTransaction.
Call GetTransaction instead, so the lookup and its error handling are
in one place.

GetTransaction checks the mempool before the txindex, so a prevout in
the mempool is now found without reading the index.

fa4723a906

Add doc/release-notes-35728.md fa28cd6503

maflcko force-pushed on Oct 30, 2026