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