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

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

<details>

    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>

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)

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

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?

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.

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

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

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.

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

thx, done

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

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.