Merge bitcoin#9853: Fix error codes from various RPCs

adaa281 Update release notes to include RPC error code changes. (John Newbery)
338bf06 Add commenting around JSON error codes (John Newbery)
dab804c Return correct error codes in fundrawtransaction(). (John Newbery)
a012087 Return correct error codes in setban(). (John Newbery)
960bc7f Return correct error codes in removeprunedfunds(). (John Newbery)
c119096 Return correct error codes in blockchain.cpp. (John Newbery)
6d07c62 Return correct error codes in bumpfee(). (John Newbery)

Tree-SHA512: 4bb39ad221cd8c83d98ac5d7ad642f3a8c265522720dc86b2eebc70e74439a85b06d6ddcd6a874e879d986511de3ab0878bb7fe58b50cb0546b78913632ea809

qa/rpc-tests/fundrawtransaction.py

@@ -187,12 +187,7 @@ def run_test(self):
         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
         assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
 
-        try:
-            self.nodes[2].fundrawtransaction(rawtx, {'foo': 'bar'})
-            raise AssertionError("Accepted invalid option foo")
-        except JSONRPCException as e:
-            assert("Unexpected key foo" in e.error['message'])
-
+        assert_raises_jsonrpc(-3, "Unexpected key foo", self.nodes[2].fundrawtransaction, rawtx, {'foo':'bar'})
 
         ############################################################
         # test a fundrawtransaction with an invalid change address #
@@ -205,12 +200,7 @@ def run_test(self):
         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
         assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
 
-        try:
-            self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': 'foobar'})
-            raise AssertionError("Accepted invalid dash address")
-        except JSONRPCException as e:
-            assert("changeAddress must be a valid dash address" in e.error['message'])
-
+        assert_raises_jsonrpc(-5, "changeAddress must be a valid dash address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'})
 
         ############################################################
         # test a fundrawtransaction with a provided change address #
@@ -224,12 +214,7 @@ def run_test(self):
         assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
 
         change = self.nodes[2].getnewaddress()
-        try:
-            rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': change, 'changePosition': 2})
-        except JSONRPCException as e:
-            assert('changePosition out of bounds' == e.error['message'])
-        else:
-            assert(False)
+        assert_raises_jsonrpc(-8, "changePosition out of bounds", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':change, 'changePosition':2})
         rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': change, 'changePosition': 0})
         dec_tx  = self.nodes[2].decoderawtransaction(rawtxfund['hex'])
         out = dec_tx['vout'][0]
@@ -338,12 +323,7 @@ def run_test(self):
         rawtx   = self.nodes[2].createrawtransaction(inputs, outputs)
         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
 
-        try:
-            rawtxfund = self.nodes[2].fundrawtransaction(rawtx)
-            raise AssertionError("Spent more than available")
-        except JSONRPCException as e:
-            assert("Insufficient" in e.error['message'])
-
+        assert_raises_jsonrpc(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx)
 
         ############################################################
         #compare fee of a standard pubkeyhash transaction
@@ -499,21 +479,13 @@ def run_test(self):
         rawTx = self.nodes[1].createrawtransaction(inputs, outputs)
         # fund a transaction that requires a new key for the change output
         # creating the key must be impossible because the wallet is locked
-        try:
-            fundedTx = self.nodes[1].fundrawtransaction(rawTx)
-            raise AssertionError("Wallet unlocked without passphrase")
-        except JSONRPCException as e:
-            assert('Keypool ran out' in e.error['message'])
+        assert_raises_jsonrpc(-4, "Insufficient funds", self.nodes[1].fundrawtransaction, rawtx)
 
         #refill the keypool
         self.nodes[1].walletpassphrase("test", 100)
         self.nodes[1].walletlock()
 
-        try:
-            self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 12)
-            raise AssertionError("Wallet unlocked without passphrase")
-        except JSONRPCException as e:
-            assert('walletpassphrase' in e.error['message'])
+        assert_raises_jsonrpc(-13, "walletpassphrase", self.nodes[1].sendtoaddress, self.nodes[0].getnewaddress(), 12)
 
         oldBalance = self.nodes[0].getbalance()
 

qa/rpc-tests/importprunedfunds.py

@@ -76,12 +76,7 @@ def run_test(self):
         self.sync_all()
 
         #Import with no affiliated address
-        try:
-            self.nodes[1].importprunedfunds(rawtxn1, proof1)
-        except JSONRPCException as e:
-            assert('No addresses' in e.error['message'])
-        else:
-            assert(False)
+        assert_raises_jsonrpc(-5, "No addresses", self.nodes[1].importprunedfunds, rawtxn1, proof1)
 
         balance1 = self.nodes[1].getbalance("", 0, False, True)
         assert_equal(balance1, Decimal(0))
@@ -112,12 +107,7 @@ def run_test(self):
         assert_equal(address_info['ismine'], True)
 
         #Remove transactions
-        try:
-            self.nodes[1].removeprunedfunds(txnid1)
-        except JSONRPCException as e:
-            assert('does not exist' in e.error['message'])
-        else:
-            assert(False)
+        assert_raises_jsonrpc(-8, "Transaction does not exist in wallet.", self.nodes[1].removeprunedfunds, txnid1)
 
         balance1 = self.nodes[1].getbalance("*", 0, False, True)
         assert_equal(balance1, Decimal('0.075'))

qa/rpc-tests/nodehandling.py

@@ -29,15 +29,13 @@ def run_test(self):
         assert_equal(len(self.nodes[2].listbanned()), 0)
         self.nodes[2].setban("127.0.0.0/24", "add")
         assert_equal(len(self.nodes[2].listbanned()), 1)
-        try:
-            self.nodes[2].setban("127.0.0.1", "add") #throws exception because 127.0.0.1 is within range 127.0.0.0/24
-        except:
-            pass
+        # This will throw an exception because 127.0.0.1 is within range 127.0.0.0/24
+        assert_raises_jsonrpc(-23, "IP/Subnet already banned", self.nodes[2].setban, "127.0.0.1", "add")
+        # This will throw an exception because 127.0.0.1/42 is not a real subnet
+        assert_raises_jsonrpc(-30, "Error: Invalid IP/Subnet", self.nodes[2].setban, "127.0.0.1/42", "add")
         assert_equal(len(self.nodes[2].listbanned()), 1) #still only one banned ip because 127.0.0.1 is within the range of 127.0.0.0/24
-        try:
-            self.nodes[2].setban("127.0.0.1", "remove")
-        except:
-            pass
+        # This will throw an exception because 127.0.0.1 was not added above
+        assert_raises_jsonrpc(-30, "Error: Unban failed", self.nodes[2].setban, "127.0.0.1", "remove")
         assert_equal(len(self.nodes[2].listbanned()), 1)
         self.nodes[2].setban("127.0.0.0/24", "remove")
         assert_equal(len(self.nodes[2].listbanned()), 0)

qa/rpc-tests/p2p-acceptblock.py

@@ -197,11 +197,8 @@ def run_test(self):
                 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)
@@ -224,28 +221,18 @@ def run_test(self):
                     headers_message.headers.append(CBlockHeader(next_block))
                 tips[j] = next_block
 
-        set_mocktime(get_mocktime() + 2)
-        set_node_times(self.nodes, get_mocktime())
-        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")
+        time.sleep(2)
+        # 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

qa/rpc-tests/pruning.py

@@ -184,11 +184,8 @@ def reorg_test(self):
 
     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 @@ def manual_test(self, node_number, use_timestamp):
         # 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 @@ def has_block(index):
             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,25 +315,19 @@ def has_block(index):
     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.
         print ("Syncing node 5 to test wallet")
         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)")

qa/rpc-tests/rawtransactions.py

@@ -61,13 +61,8 @@ def run_test(self):
         rawtx   = self.nodes[2].createrawtransaction(inputs, outputs)
         rawtx   = self.nodes[2].signrawtransaction(rawtx)
 
-        try:
-            rawtx   = self.nodes[2].sendrawtransaction(rawtx['hex'])
-        except JSONRPCException as e:
-            assert("Missing inputs" in e.error['message'])
-        else:
-            assert(False)
-
+        # This will raise an exception since there are missing inputs
+        assert_raises_jsonrpc(-25, "Missing inputs", self.nodes[2].sendrawtransaction, rawtx['hex'])
 
         #########################
         # RAW TX MULTISIG TESTS #
@@ -161,27 +156,29 @@ def run_test(self):
         assert_equal(self.nodes[0].getrawtransaction(txHash, True)["hex"], rawTxSigned['hex'])
 
         # 6. invalid parameters - supply txid and string "Flase"
-        assert_raises(JSONRPCException, self.nodes[0].getrawtransaction, txHash, "Flase")
+        assert_raises_jsonrpc(-3,"Invalid type", self.nodes[0].getrawtransaction, txHash, "Flase")
 
         # 7. invalid parameters - supply txid and empty array
-        assert_raises(JSONRPCException, self.nodes[0].getrawtransaction, txHash, [])
+        assert_raises_jsonrpc(-3,"Invalid type", self.nodes[0].getrawtransaction, txHash, [])
 
         # 8. invalid parameters - supply txid and empty dict
-        assert_raises(JSONRPCException, self.nodes[0].getrawtransaction, txHash, {})
+        assert_raises_jsonrpc(-3,"Invalid type", self.nodes[0].getrawtransaction, txHash, {})
 
         inputs  = [ {'txid' : "1d1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000", 'vout' : 1, 'sequence' : 1000}]
         outputs = { self.nodes[0].getnewaddress() : 1 }
         rawtx   = self.nodes[0].createrawtransaction(inputs, outputs)
         decrawtx= self.nodes[0].decoderawtransaction(rawtx)
         assert_equal(decrawtx['vin'][0]['sequence'], 1000)
 
+        # 9. invalid parameters - sequence number out of range
         inputs  = [ {'txid' : "1d1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000", 'vout' : 1, 'sequence' : -1}]
         outputs = { self.nodes[0].getnewaddress() : 1 }
-        assert_raises(JSONRPCException, self.nodes[0].createrawtransaction, inputs, outputs)
+        assert_raises_jsonrpc(-8, 'Invalid parameter, sequence number is out of range', self.nodes[0].createrawtransaction, inputs, outputs)
 
+        # 10. invalid parameters - sequence number out of range
         inputs  = [ {'txid' : "1d1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000", 'vout' : 1, 'sequence' : 4294967296}]
         outputs = { self.nodes[0].getnewaddress() : 1 }
-        assert_raises(JSONRPCException, self.nodes[0].createrawtransaction, inputs, outputs)
+        assert_raises_jsonrpc(-8, 'Invalid parameter, sequence number is out of range', self.nodes[0].createrawtransaction, inputs, outputs)
 
         inputs  = [ {'txid' : "1d1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000", 'vout' : 1, 'sequence' : 4294967294}]
         outputs = { self.nodes[0].getnewaddress() : 1 }

src/rpc/blockchain.cpp

@@ -908,10 +908,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 (verbosity <= 0)
     {
@@ -1006,7 +1011,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);
 
@@ -1020,15 +1025,15 @@ 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;
     }
 
     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) {

src/rpc/net.cpp

@@ -505,7 +505,7 @@ UniValue setban(const JSONRPCRequest& request)
         LookupSubNet(request.params[0].get_str().c_str(), subNet);
 
     if (! (isSubnet ? subNet.IsValid() : netAddr.IsValid()) )
-        throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: Invalid IP/Subnet");
+        throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Invalid IP/Subnet");
 
     if (strCommand == "add")
     {
@@ -525,7 +525,7 @@ UniValue setban(const JSONRPCRequest& request)
     else if(strCommand == "remove")
     {
         if (!( isSubnet ? g_connman->Unban(subNet) : g_connman->Unban(netAddr) ))
-            throw JSONRPCError(RPC_MISC_ERROR, "Error: Unban failed");
+            throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously banned.");
     }
     return NullUniValue;
 }

src/rpc/protocol.h

@@ -31,9 +31,15 @@ enum HTTPStatusCode
 enum RPCErrorCode
 {
     //! Standard JSON-RPC 2.0 errors
+    // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
+    // It should not be used for application-layer errors.
     RPC_INVALID_REQUEST  = -32600,
+    // RPC_METHOD_NOT_FOUND is internally mapped to HTTP_NOT_FOUND (404).
+    // It should not be used for application-layer errors.
     RPC_METHOD_NOT_FOUND = -32601,
     RPC_INVALID_PARAMS   = -32602,
+    // RPC_INTERNAL_ERROR should only be used for genuine errors in bitcoind
+    // (for exampled datadir corruption).
     RPC_INTERNAL_ERROR   = -32603,
     RPC_PARSE_ERROR      = -32700,