From c1190963b388590dc0a346bf625c7e84f69cee8d Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 9 Feb 2017 16:39:18 -0500 Subject: [PATCH] Return correct error codes in blockchain.cpp. RPCs in blockchain.cpp were returning misleading or incorrect error codes (for example getblock() returning RPC_INTERNAL_ERROR when the block had been pruned). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. - RPC_METHOD_NOT_FOUND should not be returned in response to a JSON request for an existing method. Those error codes have been replaced with RPC_MISC_ERROR or RPC_INVALID_PARAMETER as appropriate. --- qa/rpc-tests/p2p-acceptblock.py | 30 +++++++++--------------------- qa/rpc-tests/pruning.py | 31 +++++++++++-------------------- src/rpc/blockchain.cpp | 17 +++++++++++------ 3 files changed, 31 insertions(+), 47 deletions(-) diff --git a/qa/rpc-tests/p2p-acceptblock.py b/qa/rpc-tests/p2p-acceptblock.py index 2f21fa149..4767dd8fe 100755 --- a/qa/rpc-tests/p2p-acceptblock.py +++ b/qa/rpc-tests/p2p-acceptblock.py @@ -197,11 +197,8 @@ class AcceptBlockTest(BitcoinTestFramework): assert_equal(x['status'], "headers-only") # But this block should be accepted by node0 since it has more work. - try: - self.nodes[0].getblock(blocks_h3[0].hash) - print("Unrequested more-work block accepted from non-whitelisted peer") - except: - raise AssertionError("Unrequested more work block was not processed") + self.nodes[0].getblock(blocks_h3[0].hash) + print("Unrequested more-work block accepted from non-whitelisted peer") # Node1 should have accepted and reorged. assert_equal(self.nodes[1].getblockcount(), 3) @@ -225,26 +222,17 @@ class AcceptBlockTest(BitcoinTestFramework): tips[j] = next_block time.sleep(2) - for x in all_blocks: - try: - self.nodes[0].getblock(x.hash) - if x == all_blocks[287]: - raise AssertionError("Unrequested block too far-ahead should have been ignored") - except: - if x == all_blocks[287]: - print("Unrequested block too far-ahead not processed") - else: - raise AssertionError("Unrequested block with more work should have been accepted") + # Blocks 1-287 should be accepted, block 288 should be ignored because it's too far ahead + for x in all_blocks[:-1]: + self.nodes[0].getblock(x.hash) + assert_raises_jsonrpc(-1, "Block not found on disk", self.nodes[0].getblock, all_blocks[-1].hash) headers_message.headers.pop() # Ensure the last block is unrequested white_node.send_message(headers_message) # Send headers leading to tip white_node.send_message(msg_block(tips[1])) # Now deliver the tip - try: - white_node.sync_with_ping() - self.nodes[1].getblock(tips[1].hash) - print("Unrequested block far ahead of tip accepted from whitelisted peer") - except: - raise AssertionError("Unrequested block from whitelisted peer not accepted") + white_node.sync_with_ping() + self.nodes[1].getblock(tips[1].hash) + print("Unrequested block far ahead of tip accepted from whitelisted peer") # 5. Test handling of unrequested block on the node that didn't process # Should still not be processed (even though it has a child that has more diff --git a/qa/rpc-tests/pruning.py b/qa/rpc-tests/pruning.py index d4924d058..6d778cfc5 100755 --- a/qa/rpc-tests/pruning.py +++ b/qa/rpc-tests/pruning.py @@ -184,11 +184,8 @@ class PruneTest(BitcoinTestFramework): def reorg_back(self): # Verify that a block on the old main chain fork has been pruned away - try: - self.nodes[2].getblock(self.forkhash) - raise AssertionError("Old block wasn't pruned so can't test redownload") - except JSONRPCException as e: - print("Will need to redownload block",self.forkheight) + assert_raises_jsonrpc(-1, "Block not available (pruned data)", self.nodes[2].getblock, self.forkhash) + print("Will need to redownload block",self.forkheight) # Verify that we have enough history to reorg back to the fork point # Although this is more than 288 blocks, because this chain was written more recently @@ -233,7 +230,7 @@ class PruneTest(BitcoinTestFramework): # at this point, node has 995 blocks and has not yet run in prune mode node = self.nodes[node_number] = start_node(node_number, self.options.tmpdir, ["-debug=0"], timewait=900) assert_equal(node.getblockcount(), 995) - assert_raises_message(JSONRPCException, "not in prune mode", node.pruneblockchain, 500) + assert_raises_jsonrpc(-1, "not in prune mode", node.pruneblockchain, 500) self.stop_node(node_number) # now re-start in manual pruning mode @@ -265,14 +262,14 @@ class PruneTest(BitcoinTestFramework): return os.path.isfile(self.options.tmpdir + "/node{}/regtest/blocks/blk{:05}.dat".format(node_number, index)) # should not prune because chain tip of node 3 (995) < PruneAfterHeight (1000) - assert_raises_message(JSONRPCException, "Blockchain is too short for pruning", node.pruneblockchain, height(500)) + assert_raises_jsonrpc(-1, "Blockchain is too short for pruning", node.pruneblockchain, height(500)) # mine 6 blocks so we are at height 1001 (i.e., above PruneAfterHeight) node.generate(6) assert_equal(node.getblockchaininfo()["blocks"], 1001) # negative heights should raise an exception - assert_raises_message(JSONRPCException, "Negative", node.pruneblockchain, -10) + assert_raises_jsonrpc(-8, "Negative", node.pruneblockchain, -10) # height=100 too low to prune first block file so this is a no-op prune(100) @@ -318,12 +315,9 @@ class PruneTest(BitcoinTestFramework): def wallet_test(self): # check that the pruning node's wallet is still in good shape print("Stop and start pruning node to trigger wallet rescan") - try: - self.stop_node(2) - start_node(2, self.options.tmpdir, ["-debug=1","-prune=550"]) - print("Success") - except Exception as detail: - raise AssertionError("Wallet test: unable to re-start the pruning node") + self.stop_node(2) + start_node(2, self.options.tmpdir, ["-debug=1","-prune=550"]) + print("Success") # check that wallet loads loads successfully when restarting a pruned node after IBD. # this was reported to fail in #7494. @@ -331,12 +325,9 @@ class PruneTest(BitcoinTestFramework): connect_nodes(self.nodes[0], 5) nds = [self.nodes[0], self.nodes[5]] sync_blocks(nds, wait=5, timeout=300) - try: - self.stop_node(5) #stop and start to trigger rescan - start_node(5, self.options.tmpdir, ["-debug=1","-prune=550"]) - print ("Success") - except Exception as detail: - raise AssertionError("Wallet test: unable to re-start node5") + self.stop_node(5) #stop and start to trigger rescan + start_node(5, self.options.tmpdir, ["-debug=1","-prune=550"]) + print ("Success") def run_test(self): print("Warning! This test requires 4GB of disk space and takes over 30 mins (up to 2 hours)") diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 24ced1a81..43eec9a1f 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -745,10 +745,15 @@ UniValue getblock(const JSONRPCRequest& request) CBlockIndex* pblockindex = mapBlockIndex[hash]; if (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0) - throw JSONRPCError(RPC_INTERNAL_ERROR, "Block not available (pruned data)"); + throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)"); - if(!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) - throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk"); + if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) + // Block not found on disk. This could be because we have the block + // header in our index but don't have the block (for example if a + // non-whitelisted node sends us an unrequested long chain of valid + // blocks, we add the headers to our index, but don't accept the + // block). + throw JSONRPCError(RPC_MISC_ERROR, "Block not found on disk"); if (!fVerbose) { @@ -830,7 +835,7 @@ UniValue pruneblockchain(const JSONRPCRequest& request) + HelpExampleRpc("pruneblockchain", "1000")); if (!fPruneMode) - throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Cannot prune blocks because node is not in prune mode."); + throw JSONRPCError(RPC_MISC_ERROR, "Cannot prune blocks because node is not in prune mode."); LOCK(cs_main); @@ -844,7 +849,7 @@ UniValue pruneblockchain(const JSONRPCRequest& request) // Add a 2 hour buffer to include blocks which might have had old timestamps CBlockIndex* pindex = chainActive.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW); if (!pindex) { - throw JSONRPCError(RPC_INTERNAL_ERROR, "Could not find block with at least the specified timestamp."); + throw JSONRPCError(RPC_INVALID_PARAMETER, "Could not find block with at least the specified timestamp."); } heightParam = pindex->nHeight; } @@ -852,7 +857,7 @@ UniValue pruneblockchain(const JSONRPCRequest& request) unsigned int height = (unsigned int) heightParam; unsigned int chainHeight = (unsigned int) chainActive.Height(); if (chainHeight < Params().PruneAfterHeight()) - throw JSONRPCError(RPC_INTERNAL_ERROR, "Blockchain is too short for pruning."); + throw JSONRPCError(RPC_MISC_ERROR, "Blockchain is too short for pruning."); else if (height > chainHeight) throw JSONRPCError(RPC_INVALID_PARAMETER, "Blockchain is shorter than the attempted prune height."); else if (height > chainHeight - MIN_BLOCKS_TO_KEEP) {