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>

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