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
ACK	furszy, l0rinc, musaHaruna, ryanofsky
Stale ACK	enirox001, yuvicc, sedited

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.

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](/bitcoin-bitcoin/contributor/param/)[in]  blockman        Used to access and read blocks from disk
  * [@param](/bitcoin-bitcoin/contributor/param/)[out] hashBlock       The block hash, if the tx was found via -txindex or block_index
  * [@returns](/bitcoin-bitcoin/contributor/returns/)                    The tx if found, otherwise nullptr
+ * [@throws](/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](/bitcoin-bitcoin/contributor/param/)[in]   tx_hash  The hash of the transaction to be returned.
     /// [@return](/bitcoin-bitcoin/contributor/return/)  the transaction and containing block hash, or nullopt if it is not found
+    /// [@throws](/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.

Edit: Dropped the DrahtBot LLM linter again, because it would print stuff like:

Remove the GetName() prefix from the messages passed as err; it emits the internal index name (txindex) in the logs.

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?

ryanofsky commented at 1:13 PM on October 30, 2026:

re: #35728 (review)

Not blocking, but I'm not sure the other callers are a good reason to add the race here. getrawtransaction didn't have it before this PR, because it only checked BLOCK_HAVE_DATA after the read failed. Since this call is new, it seems better to add it in a form that doesn't race, for example by re-checking when GetTransaction throws, as in the snippet above. That is also the direction of furszy's suggestion to have ReadBlock report the kind of failure.

The race case also matters a bit more here, because it reports "I/O error reading block data" for a block that was just pruned, the kind of misreport this PR is fixing. GetBlockChecked has the same pattern but reports a neutral "Block not found on disk".

maflcko commented at 3:37 PM on October 30, 2026:

ok, pushed. I didn't like how the code looks, but I guess this may fix itself when ReadBlock's return value is refactored.

ryanofsky commented at 8:54 PM on October 30, 2026:

re: #35728 (review)

Thanks! Agree, it should get simpler once ReadBlock reports why it failed.

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

ryanofsky commented at 1:28 PM on October 30, 2026:

re: #35728 (comment)

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

Yes, this is intentional. I think a known corruption should be surfaced to the user

Agree. Now that entries left behind by an unclean shutdown are skipped, an error from the txindex here means real corruption, and reporting it is better than quietly falling through to this block read.

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.

ryanofsky commented at 8:54 PM on October 30, 2026:

re: #35728 (review)

Thanks, the new version looks good, and the other ways to reach these paths are a good point. Fine without the unit test.

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

l0rinc commented at 6:17 PM on November 2, 2026:

I also noticed that it's not strictly a refactor (the order reversal potentially changes the error), but now that I reviewed it, I don't mind either way.

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

maflcko force-pushed on Oct 30, 2026

maflcko commented at 9:19 AM on October 30, 2026: member

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.

Sure, but I don't think we have the review resources to review fixes here (and in all other tests) that are missing the sync. This is already fixed in #36618

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.

Yes, this is intentional. I think a known corruption should be surfaced to the user, so that they can test/migrate their hardware. Silently ignoring this seems a bit rude toward the user.

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

Sure, but this is a bug in the server to not throw RPC_INTERNAL_ERROR on all the internal errors like exceptions from std::vector::at, CHECK_NONFATAL, or the i/o errors, ...

maflcko force-pushed on Oct 30, 2026

DrahtBot added the label CI failed on Oct 30, 2026

maflcko force-pushed on Oct 30, 2026

in doc/release-notes-35728.md:6 in fab96992ac

   0 | @@ -0,0 +1,17 @@
   1 | +Updated RPCs
   2 | +------------
   3 | +
   4 | +- The `getrawtransaction` and `gettxoutproof` RPCs now return an internal error
   5 | +  when block files are corrupt or the transaction index is corrupt or in an
   6 | +  unclean state. Previously, they reported the transaction as missing.

ryanofsky commented at 1:13 PM on October 30, 2026:

re: #35728 (comment)

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

Sure, but this is a bug in the server to not throw RPC_INTERNAL_ERROR on all the internal errors

Agree, these are logically internal errors, and the server's catch-all mislabels them as RPC_MISC_ERROR. Until that's fixed, the release note could mention the code, so clients don't look for -32603.

Separately, "or in an unclean state" no longer fits, since entries left behind by an unclean shutdown are now skipped. Maybe:

- The `getrawtransaction` and `gettxoutproof` RPCs now return an internal error
  (reported as RPC error code -1) when block files or the transaction index
  cannot be read or are corrupt. Previously, they reported the transaction as
  missing.

The REST entry below has the same "unclean state" wording.

maflcko commented at 3:41 PM on October 30, 2026:

Sure, but this is a bug in the server to not throw RPC_INTERNAL_ERROR on all the internal errors

Agree, these are logically internal errors, and the server's catch-all mislabels them as RPC_MISC_ERROR. Until that's fixed, the release note could mention the code, so clients don't look for -32603.

heh, I still disagree. We shouldn't use release notes to cement bugs. If this is fixed, the release note will incorrectly describe the buggy behavior.

Maybe the RPC docs should describe which exceptions can be thrown, but C++ doesn't have a way to do that at compile-time ...

unclean

thx, removed unclean

ryanofsky commented at 8:54 PM on October 30, 2026:

re: #35728 (review)

Makes sense, agree it's better not to document a code we think is wrong. Thanks for removing "unclean".

in test/functional/rpc_rawtransaction.py:112 in fab96992ac outdated

 107 | +        assert_raises_rpc_error(-1, "Block not available (not fully downloaded)", node.getrawtransaction, txid="a" * 64, blockhash=block.hash_hex)
 108 | +
 109 | +        self.log.info("Test getrawtransaction when block data cannot be read")
 110 | +        blk_dat = node.blocks_path / "blk00000.dat"
 111 | +        blk_dat_moved = node.blocks_path / "blk00000.dat.moved"
 112 | +        blk_dat.rename(blk_dat_moved)

ryanofsky commented at 1:28 PM on October 30, 2026:

re: #35728 (comment)

In commit "test: Add test for getrawtransaction block read errors" (fa5ad60c32860786251afc9b764deeb9b5fdcf62)

This is already fixed in #36618

Makes sense, thanks. Waiting for index sync whenever a node starts covers this rename too.

ryanofsky approved

ryanofsky commented at 1:32 PM on October 30, 2026: contributor

Code review ACK fab96992ac470645544c21adbe48f9b5288199a1. Since my last review, 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. Thanks for taking the suggestions!

The "test ancestor commits" CI failure is the p2p_segwit.py test_uncompressed_pubkey intermittent failure in #36700, unrelated to this PR.

DrahtBot requested review from sedited on Oct 30, 2026

DrahtBot requested review from yuvicc on Oct 30, 2026

DrahtBot requested review from furszy on Oct 30, 2026

DrahtBot requested review from enirox001 on Oct 30, 2026

maflcko force-pushed on Oct 30, 2026

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

fad44f18b8

maflcko removed the label CI failed on Oct 30, 2026

maflcko added the label CI failed on Oct 30, 2026

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

maflcko force-pushed on Oct 30, 2026

maflcko commented at 3:42 PM on October 30, 2026: member

addressed all nits (including the iwyu ci)

DrahtBot removed the label CI failed on Oct 30, 2026

in src/rpc/rawtransaction.cpp:335 in fad44f18b8 outdated

 327 | @@ -327,14 +328,16 @@ static RPCMethod getrawtransaction()
 328 |      }
 329 |  
 330 |      uint256 hash_block;
 331 | -    const CTransactionRef tx = GetTransaction(blockindex, node.mempool.get(), txid, chainman.m_blockman, hash_block);
 332 | +    CTransactionRef tx;
 333 | +    try {
 334 | +        tx = GetTransaction(blockindex, node.mempool.get(), txid, chainman.m_blockman, hash_block);
 335 | +    } catch (const std::runtime_error&) {
 336 | +        if (blockindex) WITH_LOCK(::cs_main, CheckBlockDataAvailability(chainman.m_blockman, *blockindex, /*check_for_undo=*/false));

furszy commented at 8:30 PM on October 30, 2026:

nit

        if (blockindex) WITH_LOCK(chainman.GetMutext(), CheckBlockDataAvailability(chainman.m_blockman, *blockindex, /*check_for_undo=*/false));

maflcko commented at 4:12 PM on November 2, 2026:

thx, but I'll leave further nits for now, to limit the review churn :)

ryanofsky approved

ryanofsky commented at 9:15 PM on October 30, 2026: contributor

Code review ACK faa532fd36e20fab848e32d2f4d122f2fe03e885.

Since last review, getrawtransaction possible race is avoided, and release note is updated to match latest state. Thanks!

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

 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]] static void WarnAndThrowErr(const std::string& err)

l0rinc commented at 9:42 PM on November 1, 2026:

faa532f Add doc/release-notes-35728.md:

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

<details> <summary>Format warnings in the helper</summary>

diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
index 924ad045b1..268436f3e5 100644
--- a/src/index/txindex.cpp
+++ b/src/index/txindex.cpp
@@ -24,6 +24,7 @@
 #include <uint256.h>
 #include <util/fs.h>
 #include <util/log.h>
+#include <util/string.h>
 #include <validation.h>
 
 #include <algorithm>
@@ -158,8 +159,10 @@ bool TxIndex::CustomAppend(const interfaces::BlockInfo& block)
 
 BaseIndex::DB& TxIndex::GetDB() const { return *m_db; }
 
-[[noreturn]] static void WarnAndThrowErr(const std::string& err)
+template <typename... Args>
+[[noreturn]] static void WarnAndThrowErr(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
 {
+    const std::string err{tfm::format(fmt, args...)};
     LogWarning("%s", err);
     throw std::runtime_error{err};
 }
@@ -186,8 +189,8 @@ std::optional<TxIndexResult> TxIndex::FindTx(const Txid& tx_hash) const
                 // Read may throw dbwrapper_error.
                 // Read returns false for internal deserialize errors or a
                 // missing key. A missing key can only happen after corruption.
-                WarnAndThrowErr(strprintf("%s: Corrupt index: block sequence %u not found for txid %s",
-                                          GetName(), key.pos.block_seq, tx_hash.ToString()));
+                WarnAndThrowErr("%s: Corrupt index: block sequence %u not found for txid %s",
+                                GetName(), key.pos.block_seq, tx_hash.ToString());
             }
             LOCK(cs_main);
             const CBlockIndex* block_index{m_chainstate->m_blockman.LookupBlockIndex(candidate_block_hash)};
@@ -199,8 +202,8 @@ std::optional<TxIndexResult> TxIndex::FindTx(const Txid& tx_hash) const
                 // A stale entry may also refer to a known block without data.
                 // Skip it if it is not in the active chain.
                 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()));
+                WarnAndThrowErr("%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, in_active_chain);
@@ -213,15 +216,15 @@ std::optional<TxIndexResult> TxIndex::FindTx(const Txid& tx_hash) const
     for (const auto& candidate : candidates) {
         AutoFile file{m_chainstate->m_blockman.OpenBlockFile(candidate.tx_position, /*fReadOnly=*/true)};
         if (file.IsNull()) {
-            WarnAndThrowErr(strprintf("%s: Failed to open block file for txid %s at %s in block %s",
-                                      GetName(), tx_hash.ToString(), candidate.tx_position.ToString(), candidate.block_hash.ToString()));
+            WarnAndThrowErr("%s: Failed to open block file for txid %s at %s in block %s",
+                            GetName(), tx_hash.ToString(), candidate.tx_position.ToString(), candidate.block_hash.ToString());
         }
         CTransactionRef tx;
         try {
             file >> TX_WITH_WITNESS(tx);
         } catch (const std::exception& e) {
-            WarnAndThrowErr(strprintf("%s: Deserialize or I/O error (%s). txid %s at %s in block %s",
-                                      GetName(), e.what(), tx_hash.ToString(), candidate.tx_position.ToString(), candidate.block_hash.ToString()));
+            WarnAndThrowErr("%s: Deserialize or I/O error (%s). txid %s at %s in block %s",
+                            GetName(), e.what(), tx_hash.ToString(), candidate.tx_position.ToString(), candidate.block_hash.ToString());
         }
         if (tx->GetHash() == tx_hash) {
             return TxIndexResult{candidate.block_hash, std::move(tx)};
@@ -237,8 +240,8 @@ std::optional<TxIndexResult> TxIndex::FindLegacyTx(const Txid& tx_hash) const
     CDiskTxPos postx;
     const auto read{m_db->TryRead(txindex::LegacyTxKey(tx_hash), postx)};
     if (!read) {
-        WarnAndThrowErr(strprintf("%s: Failed to read legacy entry for txid %s: %s",
-                                  GetName(), tx_hash.ToString(), read.error().err_msg));
+        WarnAndThrowErr("%s: Failed to read legacy entry for txid %s: %s",
+                        GetName(), tx_hash.ToString(), read.error().err_msg);
     }
     if (!read.value()) {
         return std::nullopt;
@@ -246,8 +249,8 @@ std::optional<TxIndexResult> TxIndex::FindLegacyTx(const Txid& tx_hash) const
 
     AutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, /*fReadOnly=*/true)};
     if (file.IsNull()) {
-        WarnAndThrowErr(strprintf("%s: Failed to open block file for txid %s at %s",
-                                  GetName(), tx_hash.ToString(), postx.ToString()));
+        WarnAndThrowErr("%s: Failed to open block file for txid %s at %s",
+                        GetName(), tx_hash.ToString(), postx.ToString());
     }
     CBlockHeader header;
     CTransactionRef tx;
@@ -256,12 +259,12 @@ std::optional<TxIndexResult> TxIndex::FindLegacyTx(const Txid& tx_hash) const
         file.seek(postx.nTxOffset, SEEK_CUR);
         file >> TX_WITH_WITNESS(tx);
     } catch (const std::exception& e) {
-        WarnAndThrowErr(strprintf("%s: Deserialize or I/O error (%s). txid %s at %s",
-                                  GetName(), e.what(), tx_hash.ToString(), postx.ToString()));
+        WarnAndThrowErr("%s: Deserialize or I/O error (%s). txid %s at %s",
+                        GetName(), e.what(), tx_hash.ToString(), postx.ToString());
     }
     if (tx->GetHash() != tx_hash) {
-        WarnAndThrowErr(strprintf("%s: txid mismatch in legacy index. txid %s at %s",
-                                  GetName(), tx_hash.ToString(), postx.ToString()));
+        WarnAndThrowErr("%s: txid mismatch in legacy index. txid %s at %s",
+                        GetName(), tx_hash.ToString(), postx.ToString());
     }
     return TxIndexResult{header.GetHash(), std::move(tx)};
 }

</details>

maflcko commented at 7:03 AM on November 2, 2026:

sure, pushed.

Also:

I’ve just started reviewing this and wanted to clarify the overlap with #34132 before continuing.

The error handling here complements it, but there’s overlap in the index database reads: this PR throws on undecodable entries, while #34132 makes those failures fatal in CDBWrapper. It also removes TryRead(), which the legacy lookup here uses.

I don’t mind rebasing again if this lands first, but I’d like to clarify how we want these database-read failures handled so the two changes fit together.

I think it should be trivial to change WarnAndThrowErr to WarnAndAbort or WarnAndTriggerShutdown. Also, it should be trivial to rebase 34132 on top of this one.

I won't comment on the db error here, to keep the thread here focussed (let's do that in the respective threads). Also, I am not fixing all db errors here (ref: #35728 (comment)). So my hope is that this pull is a minimal change in the right direction. The benefits are:

io error paths (and RPC return types) are different from not-found paths. Those changes are orthogonal to 34132.
tests are added, which can help 34132 as well, as reviewers won't need to ask for them in 34132.

in doc/release-notes-35728.md:5 in faa532fd36

   0 | @@ -0,0 +1,17 @@
   1 | +Updated RPCs
   2 | +------------
   3 | +
   4 | +- The `getrawtransaction` and `gettxoutproof` RPCs now return an internal error
   5 | +  when block files are corrupt or the transaction index is corrupt.

l0rinc commented at 9:56 PM on November 1, 2026:

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?

maflcko commented at 7:03 AM on November 2, 2026:

This is just the release notes, so they should roughly be correct on a conceptual level, but I don't think we need to enumerate the exact behavior here. If we wanted to do that, the RPC docs should be used.

Still, I pushed something.

l0rinc commented at 10:08 PM on November 1, 2026: contributor

I’ve just started reviewing this and wanted to clarify the overlap with #34132 before continuing.

The error handling here complements it, but there’s overlap in the index database reads: this PR throws on undecodable entries, while #34132 makes those failures fatal in CDBWrapper. It also removes TryRead(), which the legacy lookup here uses.

I don’t mind rebasing again if this lands first, but I’d like to clarify how we want these database-read failures handled so the two changes fit together.

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.

fa32e60753

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.

fa7d278aad

Add doc/release-notes-35728.md fad49fd81f

maflcko force-pushed on Nov 2, 2026

in test/functional/feature_io_errors.py:27 in fad49fd81f

  22 | +        self.uses_wallet = None
  23 | +
  24 | +    def test_blk_dat_io_error(self):
  25 | +        node = self.nodes[0]
  26 | +        self.restart_node(0, ["-txindex", "-rest"])
  27 | +        self.wait_until(lambda: node.getindexinfo()["txindex"]["synced"])

l0rinc commented at 10:57 AM on November 2, 2026:

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

nit:

        sync_txindex(self, node)

maflcko commented at 7:22 AM on November 3, 2026:

Pretty sure sync_txindex has been removed by now. The line can just be removed

ryanofsky commented at 6:26 PM on November 3, 2026:

re: #35728 (review)

Pretty sure sync_txindex has been removed by now. The line can just be removed

Note: this is referring to #36618, which removed sync_txindex and made every node start and restart wait for index sync. After a rebase, the restart_node call above already waits, so this wait_until line is redundant but harmless. (This PR would need to be rebased to remove here.)

in test/functional/feature_io_errors.py:26 in fad49fd81f

  21 | +        self.num_nodes = 1
  22 | +        self.uses_wallet = None
  23 | +
  24 | +    def test_blk_dat_io_error(self):
  25 | +        node = self.nodes[0]
  26 | +        self.restart_node(0, ["-txindex", "-rest"])

l0rinc commented at 10:58 AM on November 2, 2026:

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

Any reason for the restart?

diff --git a/test/functional/feature_io_errors.py b/test/functional/feature_io_errors.py
index 5edd0462d6..3fff0ad44b 100755
--- a/test/functional/feature_io_errors.py
+++ b/test/functional/feature_io_errors.py
@@ -20,10 +20,10 @@ class IOErrorTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.uses_wallet = None
+        self.extra_args = [["-txindex", "-rest"]]
 
     def test_blk_dat_io_error(self):
         node = self.nodes[0]
-        self.restart_node(0, ["-txindex", "-rest"])
         self.wait_until(lambda: node.getindexinfo()["txindex"]["synced"])
         wallet = MiniWallet(node)

maflcko commented at 7:22 AM on November 3, 2026:

I think this depend on how this test evolves in the future. I can see it go either way (or another).

in test/functional/rpc_rawtransaction.py:103 in fad49fd81f

  96 | @@ -92,6 +97,23 @@ def run_test(self):
  97 |          self.transaction_version_number_tests()
  98 |          self.getrawtransaction_verbosity_tests()
  99 |  
 100 | +    def test_getrawtransaction_block_data_errors(self):
 101 | +        node = self.nodes[0]
 102 | +        self.log.info("Test getrawtransaction with a header-only block")
 103 | +        block = create_block(tmpl=node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS))

l0rinc commented at 10:58 AM on November 2, 2026:

fa5ad60 test: Add test for getrawtransaction block read errors:

nit: generateblock(..., submit=False) could be used here:

<details> <summary>Reuse the block-generation helper</summary>

diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 56492c660b..998114642a 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -18,6 +18,2 @@ from itertools import product
 
-from test_framework.blocktools import (
-    NORMAL_GBT_REQUEST_PARAMS,
-    create_block,
-)
 from test_framework.messages import (
@@ -101,7 +97,6 @@ class RawTransactionsTest(BitcoinTestFramework):
         self.log.info("Test getrawtransaction with a header-only block")
-        block = create_block(tmpl=node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS))
-        block.solve()
-        node.submitheader(block.serialize()[:80].hex())
+        block = self.generateblock(node, output="raw(55)", transactions=[], submit=False)
+        node.submitheader(block["hex"])
 
-        assert_raises_rpc_error(-1, "Block not available (not fully downloaded)", node.getrawtransaction, txid="a" * 64, blockhash=block.hash_hex)
+        assert_raises_rpc_error(-1, "Block not available (not fully downloaded)", node.getrawtransaction, txid="a" * 64, blockhash=block["hash"])

</details>

maflcko commented at 7:22 AM on November 3, 2026:

thx, will do if I have to re-touch

in test/functional/feature_io_errors.py:30 in fad49fd81f

  25 | +        node = self.nodes[0]
  26 | +        self.restart_node(0, ["-txindex", "-rest"])
  27 | +        self.wait_until(lambda: node.getindexinfo()["txindex"]["synced"])
  28 | +        wallet = MiniWallet(node)
  29 | +
  30 | +        tx = wallet.send_self_transfer(from_node=node)

l0rinc commented at 10:59 AM on November 2, 2026:

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

nit: similarly, send_self_transfer_chain(..., chain_length=2) seems simpler here

<details> <summary>Reuse the existing transaction-chain helper</summary>

diff --git a/test/functional/feature_io_errors.py b/test/functional/feature_io_errors.py
index 3fff0ad44b..fff7345729 100755
--- a/test/functional/feature_io_errors.py
+++ b/test/functional/feature_io_errors.py
@@ -27,9 +27,8 @@ class IOErrorTest(BitcoinTestFramework):
         self.wait_until(lambda: node.getindexinfo()["txindex"]["synced"])
         wallet = MiniWallet(node)
 
-        tx = wallet.send_self_transfer(from_node=node)
+        tx, tx2 = wallet.send_self_transfer_chain(from_node=node, chain_length=2)
         txid = tx["txid"]
-        tx2 = wallet.send_self_transfer(from_node=node, utxo_to_spend=tx["new_utxo"])
         self.generate(node, 1)
         psbt = node.createpsbt(inputs=[{"txid": txid, "vout": 0}], outputs=[])

</details>

maflcko commented at 7:22 AM on November 3, 2026:

thx, will do if I have to re-touch

in src/index/txindex.cpp:226 in fad49fd81f

 235 |          try {
 236 |              file >> TX_WITH_WITNESS(tx);
 237 |          } catch (const std::exception& e) {
 238 | -            LogWarning("Deserialize or I/O error - %s", e.what());
 239 | -            continue;
 240 | +            WarnAndThrowErr("%s: Deserialize or I/O error (%s). txid %s at %s in block %s",

l0rinc commented at 11:01 AM on November 2, 2026:

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

nit: I’ve bumped into these lookup paths a few times and find them easier to follow with the whole happy paths inside the try blocks. Just resolve without comment if you think it's completely unrelated, but I find this more consistent.

<details> <summary>Keep the happy paths together</summary>

diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
index 268436f3e5..7465eb82b6 100644
--- a/src/index/txindex.cpp
+++ b/src/index/txindex.cpp
@@ -219,16 +219,16 @@ std::optional<TxIndexResult> TxIndex::FindTx(const Txid& tx_hash) const
             WarnAndThrowErr("%s: Failed to open block file for txid %s at %s in block %s",
                             GetName(), tx_hash.ToString(), candidate.tx_position.ToString(), candidate.block_hash.ToString());
         }
-        CTransactionRef tx;
         try {
+            CTransactionRef tx;
             file >> TX_WITH_WITNESS(tx);
+            if (tx->GetHash() == tx_hash) {
+                return TxIndexResult{candidate.block_hash, std::move(tx)};
+            }
         } catch (const std::exception& e) {
             WarnAndThrowErr("%s: Deserialize or I/O error (%s). txid %s at %s in block %s",
                             GetName(), e.what(), tx_hash.ToString(), candidate.tx_position.ToString(), candidate.block_hash.ToString());
         }
-        if (tx->GetHash() == tx_hash) {
-            return TxIndexResult{candidate.block_hash, std::move(tx)};
-        }
     }
     // Fall back to legacy if no hashed entry matched. This makes misses pay an
     // extra lookup, but keeps existing full-txid entries readable after upgrade.
@@ -252,19 +252,19 @@ std::optional<TxIndexResult> TxIndex::FindLegacyTx(const Txid& tx_hash) const
         WarnAndThrowErr("%s: Failed to open block file for txid %s at %s",
                         GetName(), tx_hash.ToString(), postx.ToString());
     }
-    CBlockHeader header;
-    CTransactionRef tx;
     try {
+        CBlockHeader header;
+        CTransactionRef tx;
         file >> header;
         file.seek(postx.nTxOffset, SEEK_CUR);
         file >> TX_WITH_WITNESS(tx);
+        if (tx->GetHash() == tx_hash) {
+            return TxIndexResult{header.GetHash(), std::move(tx)};
+        }
     } catch (const std::exception& e) {
         WarnAndThrowErr("%s: Deserialize or I/O error (%s). txid %s at %s",
                         GetName(), e.what(), tx_hash.ToString(), postx.ToString());
     }
-    if (tx->GetHash() != tx_hash) {
-        WarnAndThrowErr("%s: txid mismatch in legacy index. txid %s at %s",
-                        GetName(), tx_hash.ToString(), postx.ToString());
-    }
-    return TxIndexResult{header.GetHash(), std::move(tx)};
+    WarnAndThrowErr("%s: txid mismatch in legacy index. txid %s at %s",
+                    GetName(), tx_hash.ToString(), postx.ToString());
 }

</details>

maflcko commented at 7:22 AM on November 3, 2026:

leaving as-is to keep it easier to review

ryanofsky commented at 6:20 PM on November 3, 2026:

re: #35728 (review)

leaving as-is to keep it easier to review

Yeah FWIW I prefer current version because it makes it intent clearer. If file.seek is only thing inside try block it makes it obvious code is trying and expecting to catch exceptions from file.seek. If tx->GetHash() and other code is moved in to the block it makes it look misleadingly like the we are expecting tx->GetHash() or that other code to throw when we aren't.

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

 201 | -                continue;
 202 | +            // Stale entries can remain after an unclean shutdown. Unknown blocks
 203 | +            // cannot be in the active chain, so skip them.
 204 | +            if (!block_index) continue;
 205 | +            const bool in_active_chain{m_chainstate->m_chain.Contains(*block_index)};
 206 | +            if (!(block_index->nStatus & BLOCK_HAVE_DATA)) {

l0rinc commented at 11:01 AM on November 2, 2026:

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

Nit: another one that is slightly unrelated: we could avoid the negation by checking the complement instead:

            if (~block_index->nStatus & BLOCK_HAVE_DATA) {

maflcko commented at 7:22 AM on November 3, 2026:

leaving as-is to keep this easier to review and consistent:

$ git grep 'nStatus & BLOCK_HAVE_DATA'|grep '!'
src/index/txindex.cpp:            if (!(block_index->nStatus & BLOCK_HAVE_DATA)) continue;
src/net_processing.cpp:        if (!(pindex->nStatus & BLOCK_HAVE_DATA)) {
src/net_processing.cpp:            if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) &&
src/net_processing.cpp:            if (m_chainman.m_blockman.IsPruneMode() && (!(pindex->nStatus & BLOCK_HAVE_DATA) || pindex->nHeight <= m_chainman.ActiveChain().Tip()->nHeight - nPrunedBlocksLikelyToHave)) {
src/net_processing.cpp:            if (!pindex || !(pindex->nStatus & BLOCK_HAVE_DATA)) {
src/node/blockstorage.cpp:    return m_have_pruned && !(block.nStatus & BLOCK_HAVE_DATA) && (block.nTx > 0);
src/node/interfaces.cpp:        return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
src/rest.cpp:        if (!(pblockindex->nStatus & BLOCK_HAVE_DATA)) {
src/test/blockmanager_tests.cpp:        while (it != nullptr && it->nStatus & BLOCK_HAVE_DATA) {
src/test/fuzz/block_index_tree.cpp:                if (prune_block != chain.Tip() && (prune_block->nStatus & BLOCK_HAVE_DATA)) {
src/test/fuzz/block_index_tree.cpp:                assert(!(index->nStatus & BLOCK_HAVE_DATA));
src/validation.cpp:            bool fMissingData = !(pindexTest->nStatus & BLOCK_HAVE_DATA);
src/validation.cpp:        if ((chainstate.m_blockman.IsPruneMode() || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
src/validation.cpp:                    if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) {
src/validation.cpp:        if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
src/validation.cpp:            assert(!(pindex->nStatus & BLOCK_HAVE_DATA) == (pindex->nTx == 0));
src/validation.cpp:        if (pindex->pprev && (pindex->nStatus & BLOCK_HAVE_DATA) && pindexFirstNeverProcessed != nullptr && pindexFirstInvalid == nullptr) {
src/validation.cpp:        if (!(pindex->nStatus & BLOCK_HAVE_DATA)) assert(!foundInUnlinked); // Can't be in m_blocks_unlinked if we don't HAVE_DATA
src/validation.cpp:        if (pindex->pprev && (pindex->nStatus & BLOCK_HAVE_DATA) && pindexFirstNeverProcessed == nullptr && pindexFirstMissing != nullptr) {

in src/rpc/rawtransaction.cpp:178 in fad49fd81f

 183 | -        }
 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;

l0rinc commented at 11:01 AM on November 2, 2026:

fa7d278 refactor: Use GetTransaction in ProcessPSBT:

Nit: was searching for where this is used - it's not, we could name it dummy_hash to make that clear.

maflcko commented at 7:22 AM on November 3, 2026:

thx, could rename to dummy_block_hash if I have to re-touch, but I think it is fine, the scope ends in 4 lines anyway.

in test/functional/rpc_rawtransaction.py:113 in fad49fd81f

 108 | +
 109 | +        self.log.info("Test getrawtransaction when block data cannot be read")
 110 | +        blk_dat = node.blocks_path / "blk00000.dat"
 111 | +        blk_dat_moved = node.blocks_path / "blk00000.dat.moved"
 112 | +        blk_dat.rename(blk_dat_moved)
 113 | +        assert_raises_rpc_error(-1, "I/O error reading block data", node.getrawtransaction, txid="a" * 64, blockhash=node.getblockhash(1))

l0rinc commented at 11:01 AM on November 2, 2026:

fa5ad60 test: Add test for getrawtransaction block read errors:

nit: to help the reviewers distinguish desired behavior from assertions that will be changed later, we could add TODOs to expectations that will change in follow-up commits.

<details> <summary>Mark the expectations changed by the fix</summary>

diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 9c968a79d8..ceca1d76e2 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -104,13 +104,13 @@ class RawTransactionsTest(BitcoinTestFramework):
         block.solve()
         node.submitheader(block.serialize()[:80].hex())
 
-        assert_raises_rpc_error(-1, "Block not available", node.getrawtransaction, txid="a" * 64, blockhash=block.hash_hex)
+        assert_raises_rpc_error(-1, "Block not available", node.getrawtransaction, txid="a" * 64, blockhash=block.hash_hex)  # TODO: Explain that the block has not been downloaded
 
         self.log.info("Test getrawtransaction when block data cannot be read")
         blk_dat = node.blocks_path / "blk00000.dat"
         blk_dat_moved = node.blocks_path / "blk00000.dat.moved"
         blk_dat.rename(blk_dat_moved)
-        assert_raises_rpc_error(-5, "No such transaction found in the provided block. Use gettransaction for wallet transactions.", node.getrawtransaction, txid="a" * 64, blockhash=node.getblockhash(1))
+        assert_raises_rpc_error(-5, "No such transaction found in the provided block. Use gettransaction for wallet transactions.", node.getrawtransaction, txid="a" * 64, blockhash=node.getblockhash(1))  # TODO: Do not report a missing transaction when the block could not be read
         blk_dat_moved.rename(blk_dat)

</details>

maflcko commented at 7:22 AM on November 3, 2026:

I don't like TODOs. I think an issue or a pull request is better (and this is already a pull request :sweat_smile: )

l0rinc commented at 9:37 AM on November 3, 2026:

Ah, no, I mean for code that is added in one commit and changed in the next one (to guide the reviewer through the effects of the change) - i.e. forward pointers. It can help reviewers know that the currently documented state is not final or desired, the assertions are just meant to make the current state clear.

furszy commented at 3:59 PM on November 2, 2026: member

ACK fad49fd81fcffd1dd9f40320a239881a0881be25

DrahtBot requested review from ryanofsky on Nov 2, 2026

in src/rpc/rawtransaction.cpp:327 in fad49fd81f

 319 | @@ -327,14 +320,16 @@ static RPCMethod getrawtransaction()
 320 |      }
 321 |  
 322 |      uint256 hash_block;
 323 | -    const CTransactionRef tx = GetTransaction(blockindex, node.mempool.get(), txid, chainman.m_blockman, hash_block);
 324 | +    CTransactionRef tx;
 325 | +    try {
 326 | +        tx = GetTransaction(blockindex, node.mempool.get(), txid, chainman.m_blockman, hash_block);
 327 | +    } catch (const std::runtime_error&) {
 328 | +        if (blockindex) WITH_LOCK(::cs_main, CheckBlockDataAvailability(chainman.m_blockman, *blockindex, /*check_for_undo=*/false));

l0rinc commented at 6:23 PM on November 2, 2026:

fad44f1 rpc: Return proper error messages from getrawtransaction:

nit: the commit message still says “check block availability up front”, but the check is in the catch now.

maflcko commented at 7:22 AM on November 3, 2026:

thx, will remove up front if I have to re-touch

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

  33 | +        self.generate(node, 1)
  34 | +        psbt = node.createpsbt(inputs=[{"txid": txid, "vout": 0}], outputs=[])
  35 | +
  36 | +        blk_dat = node.blocks_path / "blk00000.dat"
  37 | +        blk_dat_moved = node.blocks_path / "blk00000.dat.moved"
  38 | +        blk_dat.rename(blk_dat_moved)

l0rinc commented at 6:23 PM on November 2, 2026:

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

We could probably avoid the startup txindex race here and simplify a bit:

<details> <summary>Share one block-file failure window</summary>

diff --git a/test/functional/feature_io_errors.py b/test/functional/feature_io_errors.py
index 39fc97df70..5edd0462d6 100755
--- a/test/functional/feature_io_errors.py
+++ b/test/functional/feature_io_errors.py
@@ -40,6 +40,7 @@ class IOErrorTest(BitcoinTestFramework):
         self.log.info("Check tx RPCs throw on I/O errors")
         msg = "Can't read block from disk"
         assert_raises_rpc_error(-32603, msg, node.gettxoutproof, [tx2["txid"]])
+        assert_raises_rpc_error(-1, "I/O error reading block data", node.getrawtransaction, txid="a" * 64, blockhash=node.getblockhash(1))
         msg = "txindex: Failed to open block file"
         assert_raises_rpc_error(-1, msg, node.gettxoutproof, [txid])
         assert_raises_rpc_error(-1, msg, node.getrawtransaction, txid)
diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 9a009f0833..56492c660b 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -105,13 +105,6 @@ class RawTransactionsTest(BitcoinTestFramework):
 
         assert_raises_rpc_error(-1, "Block not available (not fully downloaded)", node.getrawtransaction, txid="a" * 64, blockhash=block.hash_hex)
 
-        self.log.info("Test getrawtransaction when block data cannot be read")
-        blk_dat = node.blocks_path / "blk00000.dat"
-        blk_dat_moved = node.blocks_path / "blk00000.dat.moved"
-        blk_dat.rename(blk_dat_moved)
-        assert_raises_rpc_error(-1, "I/O error reading block data", node.getrawtransaction, txid="a" * 64, blockhash=node.getblockhash(1))
-        blk_dat_moved.rename(blk_dat)
-
 
     def getrawtransaction_tests(self):
         tx = self.wallet.send_self_transfer(from_node=self.nodes[0])

</details>

maflcko commented at 7:22 AM on November 3, 2026:

I wanted to show the different outcomes of getrawtransaction on a header vs missing block file next to each other. So I think I'll leave as-is

l0rinc approved

l0rinc commented at 6:29 PM on November 2, 2026: contributor

ACK fad49fd81fcffd1dd9f40320a239881a0881be25

Only left some simplification nits, happy to re-ACK if you pick any of them up, but I’m also fine with merging as-is.

maflcko commented at 7:24 AM on November 3, 2026: member

(I'll leave the test nits for the future)

musaHaruna commented at 9:48 AM on November 3, 2026: contributor

re-ACK fad49fd

Reviewed the range-diff since my previous ACK at fa87067. The updated lookup skips leftover txindex entries referring to unknown blocks or non-active blocks without data. Missing active-chain data and failures to open or read a file still produce errors.

Checking block availability after GetTransaction fails also handles the pruning race: if the block was pruned before it could be read, the RPC reports that it was pruned instead of returning a generic I/O error.

The added REST error coverage, clearer diagnostics, formatting-helper cleanup, and release-note changes look good.

Happy to re-ACK if this is retouched, or to open a follow-up PR right after merge to address some of the remaining review comments.

ryanofsky assigned ryanofsky on Nov 3, 2026

ryanofsky approved

ryanofsky commented at 6:30 PM on November 3, 2026: contributor

Code review ACK fad49fd81fcffd1dd9f40320a239881a0881be25. Since last review, WarnAndThrowErr takes the format string and arguments directly, and the release note wording is updated. Thanks!

ryanofsky merged this on Nov 3, 2026

ryanofsky closed this on Nov 3, 2026

maflcko deleted the branch on Nov 3, 2026

maflcko commented at 9:10 PM on November 3, 2026: member

I rolled the test fixups into #36748 (along with other sync fixups I carried around). The other two remaining follow-ups would be:

Maybe ReadBlock could report why it failed (ref #35728 (review)). However, I haven't looked into this, so I am not sure if the overall diff and benefit would be worth the review effort?
Maybe database errors could be handled? Ref: #35728 (comment). However, I haven't looked into this either, and I am not sure if this is worth it. Maybe #34132 can be done first, if relevant?