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

Author: laanwj

doc/release-notes.md

@@ -33,6 +33,55 @@ frequently tested on them.
 Notable changes
 ===============
 
+Low-level RPC changes
+---------------------
+
+- Error codes have been updated to be more accurate for the following error cases:
+  - `getblock` now returns RPC_MISC_ERROR if the block can't be found on disk (for
+  example if the block has been pruned). Previously returned RPC_INTERNAL_ERROR.
+  - `pruneblockchain` now returns RPC_MISC_ERROR if the blocks cannot be pruned
+  because the node is not in pruned mode. Previously returned RPC_METHOD_NOT_FOUND.
+  - `pruneblockchain` now returns RPC_INVALID_PARAMETER if the blocks cannot be pruned
+  because the supplied timestamp is too late. Previously returned RPC_INTERNAL_ERROR.
+  - `pruneblockchain` now returns RPC_MISC_ERROR if the blocks cannot be pruned
+  because the blockchain is too short. Previously returned RPC_INTERNAL_ERROR.
+  - `setban` now returns RPC_CLIENT_INVALID_IP_OR_SUBNET if the supplied IP address
+  or subnet is invalid. Previously returned RPC_CLIENT_NODE_ALREADY_ADDED.
+  - `setban` now returns RPC_CLIENT_INVALID_IP_OR_SUBNET if the user tries to unban
+  a node that has not previously been banned. Previously returned RPC_MISC_ERROR.
+  - `removeprunedfunds` now returns RPC_WALLET_ERROR if bitcoind is unable to remove
+  the transaction. Previously returned RPC_INTERNAL_ERROR.
+  - `removeprunedfunds` now returns RPC_INVALID_PARAMETER if the transaction does not
+  exist in the wallet. Previously returned RPC_INTERNAL_ERROR.
+  - `fundrawtransaction` now returns RPC_INVALID_ADDRESS_OR_KEY if an invalid change
+  address is provided. Previously returned RPC_INVALID_PARAMETER.
+  - `fundrawtransaction` now returns RPC_WALLET_ERROR if bitcoind is unable to create
+  the transaction. The error message provides further details. Previously returned
+  RPC_INTERNAL_ERROR.
+  - `bumpfee` now returns RPC_INVALID_PARAMETER if the provided transaction has
+  descendants in the wallet. Previously returned RPC_MISC_ERROR.
+  - `bumpfee` now returns RPC_INVALID_PARAMETER if the provided transaction has
+  descendants in the mempool. Previously returned RPC_MISC_ERROR.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the provided transaction has
+  has been mined or conflicts with a mined transaction. Previously returned
+  RPC_INVALID_ADDRESS_OR_KEY.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the provided transaction is not
+  BIP 125 replaceable. Previously returned RPC_INVALID_ADDRESS_OR_KEY.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the provided transaction has already
+  been bumped by a different transaction. Previously returned RPC_INVALID_REQUEST.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the provided transaction contains
+  inputs which don't belong to this wallet. Previously returned RPC_INVALID_ADDRESS_OR_KEY.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the provided transaction has multiple change
+  outputs. Previously returned RPC_MISC_ERROR.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the provided transaction has no change
+  output. Previously returned RPC_MISC_ERROR.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the fee is too high. Previously returned
+  RPC_MISC_ERROR.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the fee is too low. Previously returned
+  RPC_MISC_ERROR.
+  - `bumpfee` now returns RPC_WALLET_ERROR if the change output is too small to bump the
+  fee. Previously returned RPC_MISC_ERROR.
+
 Credits
 =======
 

qa/rpc-tests/bumpfee.py

@@ -128,7 +128,7 @@ def test_segwit_bumpfee_succeeds(rbf_node, dest_address):
 def test_nonrbf_bumpfee_fails(peer_node, dest_address):
     # cannot replace a non RBF transaction (from node which did not enable RBF)
     not_rbfid = create_fund_sign_send(peer_node, {dest_address: 0.00090000})
-    assert_raises_message(JSONRPCException, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
+    assert_raises_jsonrpc(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
 
 
 def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address):
@@ -148,7 +148,7 @@ def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address):
     signedtx = rbf_node.signrawtransaction(rawtx)
     signedtx = peer_node.signrawtransaction(signedtx["hex"])
     rbfid = rbf_node.sendrawtransaction(signedtx["hex"])
-    assert_raises_message(JSONRPCException, "Transaction contains inputs that don't belong to this wallet",
+    assert_raises_jsonrpc(-4, "Transaction contains inputs that don't belong to this wallet",
                           rbf_node.bumpfee, rbfid)
 
 
@@ -159,7 +159,7 @@ def test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address)
     tx = rbf_node.createrawtransaction([{"txid": parent_id, "vout": 0}], {dest_address: 0.00020000})
     tx = rbf_node.signrawtransaction(tx)
     txid = rbf_node.sendrawtransaction(tx["hex"])
-    assert_raises_message(JSONRPCException, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
+    assert_raises_jsonrpc(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
 
 
 def test_small_output_fails(rbf_node, dest_address):
@@ -174,7 +174,7 @@ def test_small_output_fails(rbf_node, dest_address):
                             Decimal("0.00100000"),
                             {dest_address: 0.00080000,
                              get_change_address(rbf_node): Decimal("0.00010000")})
-    assert_raises_message(JSONRPCException, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 20001})
+    assert_raises_jsonrpc(-4, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 20001})
 
 
 def test_dust_to_fee(rbf_node, dest_address):
@@ -209,15 +209,15 @@ def test_rebumping(rbf_node, dest_address):
     rbf_node.settxfee(Decimal("0.00001000"))
     rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000})
     bumped = rbf_node.bumpfee(rbfid, {"totalFee": 1000})
-    assert_raises_message(JSONRPCException, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 2000})
+    assert_raises_jsonrpc(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 2000})
     rbf_node.bumpfee(bumped["txid"], {"totalFee": 2000})
 
 
 def test_rebumping_not_replaceable(rbf_node, dest_address):
     # check that re-bumping a non-replaceable bump tx fails
     rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000})
     bumped = rbf_node.bumpfee(rbfid, {"totalFee": 10000, "replaceable": False})
-    assert_raises_message(JSONRPCException, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"],
+    assert_raises_jsonrpc(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"],
                           {"totalFee": 20000})
 
 
@@ -268,7 +268,7 @@ def test_bumpfee_metadata(rbf_node, dest_address):
 def test_locked_wallet_fails(rbf_node, dest_address):
     rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000})
     rbf_node.walletlock()
-    assert_raises_message(JSONRPCException, "Please enter the wallet passphrase with walletpassphrase first.",
+    assert_raises_jsonrpc(-13, "Please enter the wallet passphrase with walletpassphrase first.",
                           rbf_node.bumpfee, rbfid)
 
 
@@ -315,9 +315,7 @@ def submit_block_with_tx(node, tx):
     block.rehash()
     block.hashMerkleRoot = block.calc_merkle_root()
     block.solve()
-    error = node.submitblock(bytes_to_hex_str(block.serialize(True)))
-    if error is not None:
-        raise Exception(error)
+    node.submitblock(bytes_to_hex_str(block.serialize(True)))
     return block
 
 

qa/rpc-tests/fundrawtransaction.py

@@ -182,12 +182,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 #
@@ -200,12 +195,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 bitcoin address")
-        except JSONRPCException as e:
-            assert("changeAddress must be a valid bitcoin address" in e.error['message'])
-
+        assert_raises_jsonrpc(-5, "changeAddress must be a valid bitcoin address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'})
 
         ############################################################
         # test a fundrawtransaction with a provided change address #
@@ -219,12 +209,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]
@@ -333,12 +318,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
@@ -494,21 +474,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(), 1.2)
-            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(), 1.2)
 
         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, 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, 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)
@@ -225,26 +222,17 @@ def run_test(self):
                 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

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