From 6a1aa510e31e8b77793341473aa5afc9d023a6e3 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 16 Jul 2024 16:30:02 -0400 Subject: [PATCH] rpc: check block index before reading block / undo data This avoids low-level log errors that are supposed to only occur when there is an actual problem with the block on disk missing unexpectedly, but not in the case where the block and/or undo data are expected not to be there. It changes behavior such that in the first case (block index indicates data is available but retrieving it fails) an error is thrown. It also adjusts a functional tests that tried to simulate not having undo data (but having block data) by deleting the undo file. This situation should occur reality because block and undo data are pruned together. Instead, test this situation with a block that hasn't been connected. --- src/rpc/blockchain.cpp | 6 ++-- src/rpc/rawtransaction.cpp | 9 ++++-- test/functional/rpc_blockchain.py | 48 +++++++++++++++++++------------ 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 0dc1dca89c0..a9cea44779d 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -201,8 +201,10 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn case TxVerbosity::SHOW_DETAILS_AND_PREVOUT: CBlockUndo blockUndo; const bool is_not_pruned{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(blockindex))}; - const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, blockindex)}; - + bool have_undo{is_not_pruned && WITH_LOCK(::cs_main, return blockindex.nStatus & BLOCK_HAVE_UNDO)}; + if (have_undo && !blockman.UndoReadFromDisk(blockUndo, blockindex)) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "Undo data expected but can't be read. This could be due to disk corruption or a conflict with a pruning event."); + } for (size_t i = 0; i < block.vtx.size(); ++i) { const CTransactionRef& tx = block.vtx.at(i); // coinbase transaction (i.e. i == 0) doesn't have undo data diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 21bc0e52f13..bb072370ce1 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -405,11 +405,16 @@ static RPCHelpMan getrawtransaction() CBlockUndo blockUndo; CBlock block; - if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return chainman.m_blockman.IsBlockPruned(*blockindex)) || - !(chainman.m_blockman.UndoReadFromDisk(blockUndo, *blockindex) && chainman.m_blockman.ReadBlockFromDisk(block, *blockindex))) { + if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return !(blockindex->nStatus & BLOCK_HAVE_MASK))) { TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); return result; } + if (!chainman.m_blockman.UndoReadFromDisk(blockUndo, *blockindex)) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "Undo data expected but can't be read. This could be due to disk corruption or a conflict with a pruning event."); + } + if (!chainman.m_blockman.ReadBlockFromDisk(block, *blockindex)) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "Block data expected but can't be read. This could be due to disk corruption or a conflict with a pruning event."); + } CTxUndo* undoTX {nullptr}; auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return *t == *tx; }); diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 5a91cab59e1..91e9975ad54 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -32,14 +32,16 @@ from test_framework.blocktools import ( TIME_GENESIS_BLOCK, create_block, create_coinbase, + create_tx_with_script, ) from test_framework.messages import ( CBlockHeader, + COIN, from_hex, msg_block, ) from test_framework.p2p import P2PInterface -from test_framework.script import hash256 +from test_framework.script import hash256, OP_TRUE from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -556,12 +558,12 @@ class BlockchainTest(BitcoinTestFramework): block = node.getblock(blockhash, verbosity) assert_equal(blockhash, hash256(bytes.fromhex(block[:160]))[::-1].hex()) - def assert_fee_not_in_block(verbosity): - block = node.getblock(blockhash, verbosity) + def assert_fee_not_in_block(hash, verbosity): + block = node.getblock(hash, verbosity) assert 'fee' not in block['tx'][1] - def assert_fee_in_block(verbosity): - block = node.getblock(blockhash, verbosity) + def assert_fee_in_block(hash, verbosity): + block = node.getblock(hash, verbosity) tx = block['tx'][1] assert 'fee' in tx assert_equal(tx['fee'], tx['vsize'] * fee_per_byte) @@ -580,8 +582,8 @@ class BlockchainTest(BitcoinTestFramework): total_vout += vout["value"] assert_equal(total_vin, total_vout + tx["fee"]) - def assert_vin_does_not_contain_prevout(verbosity): - block = node.getblock(blockhash, verbosity) + def assert_vin_does_not_contain_prevout(hash, verbosity): + block = node.getblock(hash, verbosity) tx = block["tx"][1] if isinstance(tx, str): # In verbosity level 1, only the transaction hashes are written @@ -595,16 +597,16 @@ class BlockchainTest(BitcoinTestFramework): assert_hexblock_hashes(False) self.log.info("Test that getblock with verbosity 1 doesn't include fee") - assert_fee_not_in_block(1) - assert_fee_not_in_block(True) + assert_fee_not_in_block(blockhash, 1) + assert_fee_not_in_block(blockhash, True) self.log.info('Test that getblock with verbosity 2 and 3 includes expected fee') - assert_fee_in_block(2) - assert_fee_in_block(3) + assert_fee_in_block(blockhash, 2) + assert_fee_in_block(blockhash, 3) self.log.info("Test that getblock with verbosity 1 and 2 does not include prevout") - assert_vin_does_not_contain_prevout(1) - assert_vin_does_not_contain_prevout(2) + assert_vin_does_not_contain_prevout(blockhash, 1) + assert_vin_does_not_contain_prevout(blockhash, 2) self.log.info("Test that getblock with verbosity 3 includes prevout") assert_vin_contains_prevout(3) @@ -612,7 +614,7 @@ class BlockchainTest(BitcoinTestFramework): self.log.info("Test getblock with invalid verbosity type returns proper error message") assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", node.getblock, blockhash, "2") - self.log.info("Test that getblock with verbosity 2 and 3 still works with pruned Undo data") + self.log.info("Test that getblock doesn't work with deleted Undo data") def move_block_file(old, new): old_path = self.nodes[0].blocks_path / old @@ -622,10 +624,8 @@ class BlockchainTest(BitcoinTestFramework): # Move instead of deleting so we can restore chain state afterwards move_block_file('rev00000.dat', 'rev_wrong') - assert_fee_not_in_block(2) - assert_fee_not_in_block(3) - assert_vin_does_not_contain_prevout(2) - assert_vin_does_not_contain_prevout(3) + assert_raises_rpc_error(-32603, "Undo data expected but can't be read. This could be due to disk corruption or a conflict with a pruning event.", lambda: node.getblock(blockhash, 2)) + assert_raises_rpc_error(-32603, "Undo data expected but can't be read. This could be due to disk corruption or a conflict with a pruning event.", lambda: node.getblock(blockhash, 3)) # Restore chain state move_block_file('rev_wrong', 'rev00000.dat') @@ -641,6 +641,18 @@ class BlockchainTest(BitcoinTestFramework): node.submitheader(block.serialize().hex()) assert_raises_rpc_error(-1, "Block not available (not fully downloaded)", lambda: node.getblock(block.hash)) + self.log.info("Test getblock when block data is available but undo data isn't") + # Submits a block building on the header-only block, so it can't be connected and has no undo data + tx = create_tx_with_script(block.vtx[0], 0, script_sig=bytes([OP_TRUE]), amount=50 * COIN) + block_noundo = create_block(block.sha256, create_coinbase(current_height + 2, nValue=100), block_time + 1, txlist=[tx]) + block_noundo.solve() + node.submitblock(block_noundo.serialize().hex()) + + assert_fee_not_in_block(block_noundo.hash, 2) + assert_fee_not_in_block(block_noundo.hash, 3) + assert_vin_does_not_contain_prevout(block_noundo.hash, 2) + assert_vin_does_not_contain_prevout(block_noundo.hash, 3) + self.log.info("Test getblock when block is missing") move_block_file('blk00000.dat', 'blk00000.dat.bak') assert_raises_rpc_error(-1, "Block not found on disk", node.getblock, blockhash)