Merge bitcoin#10853: [tests] Fix RPC failure testing (again)

47ba8cf scripted-diff: rename assert_raises_jsonrpc to assert_raises_rpc error (John Newbery)
677d893 [tests] do not allow assert_raises_message to be called with JSONRPCException (John Newbery)
5864e9c [tests] remove direct testing on JSONRPCException from individual test cases (John Newbery)

Pull request description:

  I did this a few months ago (here: bitcoin#9707), but a few new examples have crept back in.

  When testing RPC failures, the test case should always assert the error value and message, to ensure that the failure was for the correct reason. Not doing that can hide bugs in the test code and mean that the test is not testing the correct behaviour.

  RPC failure testing should use the utility function `assert_raises_jsonrpc()` (renamed in the final commit of this PR to `assert_raises_rpc_error()`.

  This PR does the following:
  - changes all remaining instances of tests directly testing on `JSONRPCException` to calls to `assert_raises_jsonrpc()`
  - prevents `assert_raises_message()` from being called with `JSONRPCException`
  - scripted-diff changes `assert_raises_jsonrpc()` to `assert_raises_rpc_error()`

Tree-SHA512: 2cc5e320704ec623a6e5a27d3c2c81cea86b502e29896f03bb5bf92cc36725132c1144410aecdf49e90d4577d512ee467d50d8184e9d5c5d0870bfc931316a5a

test/functional/bip68-sequence.py

@@ -83,7 +83,7 @@ def test_disable_flag(self):
         tx2.vout = [CTxOut(int(value-self.relayfee*COIN), CScript([b'a']))]
         tx2.rehash()
 
-        assert_raises_jsonrpc(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, ToHex(tx2))
+        assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, ToHex(tx2))
 
         # Setting the version back down to 1 should disable the sequence lock,
         # so this should be accepted.
@@ -180,7 +180,7 @@ def test_sequence_lock_confirmed_inputs(self):
 
             if (using_sequence_locks and not should_pass):
                 # This transaction should be rejected
-                assert_raises_jsonrpc(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, rawtx)
+                assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, rawtx)
             else:
                 # This raw transaction should be accepted
                 self.nodes[0].sendrawtransaction(rawtx)
@@ -227,7 +227,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
 
             if (orig_tx.hash in node.getrawmempool()):
                 # sendrawtransaction should fail if the tx is in the mempool
-                assert_raises_jsonrpc(-26, NOT_FINAL_ERROR, node.sendrawtransaction, ToHex(tx))
+                assert_raises_rpc_error(-26, NOT_FINAL_ERROR, node.sendrawtransaction, ToHex(tx))
             else:
                 # sendrawtransaction should succeed if the tx is not in the mempool
                 node.sendrawtransaction(ToHex(tx))
@@ -280,7 +280,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
         tx5.vout[0].nValue += int(utxos[0]["amount"]*COIN)
         raw_tx5 = self.nodes[0].signrawtransaction(ToHex(tx5))["hex"]
 
-        assert_raises_jsonrpc(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, raw_tx5)
+        assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, raw_tx5)
 
         # Test mempool-BIP68 consistency after reorg
         #
@@ -353,7 +353,7 @@ def test_bip68_not_consensus(self):
         tx3.vout = [CTxOut(int(tx2.vout[0].nValue - self.relayfee*COIN), CScript([b'a']))]
         tx3.rehash()
 
-        assert_raises_jsonrpc(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, ToHex(tx3))
+        assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.nodes[0].sendrawtransaction, ToHex(tx3))
 
         # make a block that violates bip68; ensure that the tip updates
         tip = int(self.nodes[0].getbestblockhash(), 16)

test/functional/blockchain.py

@@ -27,7 +27,7 @@
     assert_greater_than,
     assert_greater_than_or_equal,
     assert_raises,
-    assert_raises_jsonrpc,
+    assert_raises_rpc_error,
     assert_is_hex_string,
     assert_is_hash_string,
 )
@@ -125,7 +125,7 @@ def _test_getchaintxstats(self):
         assert('window_interval' not in chaintxstats)
         assert('txrate' not in chaintxstats)
 
-        assert_raises_jsonrpc(-8, "Invalid block count: should be between 0 and the block's height - 1", self.nodes[0].getchaintxstats, 201)
+        assert_raises_rpc_error(-8, "Invalid block count: should be between 0 and the block's height - 1", self.nodes[0].getchaintxstats, 201)
 
     def _test_gettxoutsetinfo(self):
         node = self.nodes[0]
@@ -171,7 +171,7 @@ def _test_gettxoutsetinfo(self):
     def _test_getblockheader(self):
         node = self.nodes[0]
 
-        assert_raises_jsonrpc(-5, "Block not found",
+        assert_raises_rpc_error(-5, "Block not found",
                               node.getblockheader, "nonsense")
 
         besthash = node.getbestblockhash()

test/functional/bumpfee.py

@@ -133,7 +133,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 = peer_node.sendtoaddress(dest_address, Decimal("0.00090000"))
-    assert_raises_jsonrpc(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
+    assert_raises_rpc_error(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
 
 
 def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address):
@@ -153,7 +153,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_jsonrpc(-4, "Transaction contains inputs that don't belong to this wallet",
+    assert_raises_rpc_error(-4, "Transaction contains inputs that don't belong to this wallet",
                           rbf_node.bumpfee, rbfid)
 
 
@@ -164,7 +164,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)
     rbf_node.sendrawtransaction(tx["hex"])
-    assert_raises_jsonrpc(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
+    assert_raises_rpc_error(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
 
 
 def test_small_output_fails(rbf_node, dest_address):
@@ -173,7 +173,7 @@ def test_small_output_fails(rbf_node, dest_address):
     rbf_node.bumpfee(rbfid, {"totalFee": 50000})
 
     rbfid = spend_one_input(rbf_node, dest_address)
-    assert_raises_jsonrpc(-4, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 50001})
+    assert_raises_rpc_error(-4, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 50001})
 
 
 def test_dust_to_fee(rbf_node, dest_address):
@@ -205,15 +205,15 @@ def test_rebumping(rbf_node, dest_address):
     # check that re-bumping the original tx fails, but bumping the bumper succeeds
     rbfid = spend_one_input(rbf_node, dest_address)
     bumped = rbf_node.bumpfee(rbfid, {"totalFee": 2000})
-    assert_raises_jsonrpc(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 3000})
+    assert_raises_rpc_error(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 3000})
     rbf_node.bumpfee(bumped["txid"], {"totalFee": 3000})
 
 
 def test_rebumping_not_replaceable(rbf_node, dest_address):
     # check that re-bumping a non-replaceable bump tx fails
     rbfid = spend_one_input(rbf_node, dest_address)
     bumped = rbf_node.bumpfee(rbfid, {"totalFee": 10000, "replaceable": False})
-    assert_raises_jsonrpc(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"],
+    assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"],
                           {"totalFee": 20000})
 
 
@@ -264,7 +264,7 @@ def test_bumpfee_metadata(rbf_node, dest_address):
 def test_locked_wallet_fails(rbf_node, dest_address):
     rbfid = spend_one_input(rbf_node, dest_address)
     rbf_node.walletlock()
-    assert_raises_jsonrpc(-13, "Please enter the wallet passphrase with walletpassphrase first.",
+    assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first.",
                           rbf_node.bumpfee, rbfid)
 
 

test/functional/deprecated_rpc.py

@@ -4,7 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test deprecation of RPC calls."""
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_raises_jsonrpc
+from test_framework.util import assert_raises_rpc_error
 
 class DeprecatedRpcTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -14,7 +14,7 @@ def set_test_params(self):
 
     def run_test(self):
         self.log.info("estimatefee: Shows deprecated message")
-        assert_raises_jsonrpc(-32, 'estimatefee is deprecated', self.nodes[0].estimatefee, 1)
+        assert_raises_rpc_error(-32, 'estimatefee is deprecated', self.nodes[0].estimatefee, 1)
 
         self.log.info("Using -deprecatedrpc=estimatefee bypasses the error")
         self.nodes[1].estimatefee(1)

test/functional/disablewallet.py

@@ -19,7 +19,7 @@ def set_test_params(self):
 
     def run_test (self):
         # Make sure wallet is really disabled
-        assert_raises_jsonrpc(-32601, 'Method not found', self.nodes[0].getwalletinfo)
+        assert_raises_rpc_error(-32601, 'Method not found', self.nodes[0].getwalletinfo)
         x = self.nodes[0].validateaddress('3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
         assert(x['isvalid'] == False)
         x = self.nodes[0].validateaddress('mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ')
@@ -28,7 +28,7 @@ def run_test (self):
         # Checking mining to an address without a wallet. Generating to a valid address should succeed
         # but generating to an invalid address will fail.
         self.nodes[0].generatetoaddress(1, 'mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ')
-        assert_raises_jsonrpc(-5, "Invalid address", self.nodes[0].generatetoaddress, 1, '3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
+        assert_raises_rpc_error(-5, "Invalid address", self.nodes[0].generatetoaddress, 1, '3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
 
 if __name__ == '__main__':
     DisableWalletTest ().main ()

test/functional/disconnect_ban.py

@@ -8,7 +8,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
-    assert_raises_jsonrpc,
+    assert_raises_rpc_error,
     connect_nodes_bi,
     wait_until,
 )
@@ -34,14 +34,14 @@ def run_test(self):
 
         self.log.info("setban: fail to ban an already banned subnet")
         assert_equal(len(self.nodes[1].listbanned()), 1)
-        assert_raises_jsonrpc(-23, "IP/Subnet already banned", self.nodes[1].setban, "127.0.0.1", "add")
+        assert_raises_rpc_error(-23, "IP/Subnet already banned", self.nodes[1].setban, "127.0.0.1", "add")
 
         self.log.info("setban: fail to ban an invalid subnet")
-        assert_raises_jsonrpc(-30, "Error: Invalid IP/Subnet", self.nodes[1].setban, "127.0.0.1/42", "add")
+        assert_raises_rpc_error(-30, "Error: Invalid IP/Subnet", self.nodes[1].setban, "127.0.0.1/42", "add")
         assert_equal(len(self.nodes[1].listbanned()), 1)  # still only one banned ip because 127.0.0.1 is within the range of 127.0.0.0/24
 
         self.log.info("setban remove: fail to unban a non-banned subnet")
-        assert_raises_jsonrpc(-30, "Error: Unban failed", self.nodes[1].setban, "127.0.0.1", "remove")
+        assert_raises_rpc_error(-30, "Error: Unban failed", self.nodes[1].setban, "127.0.0.1", "remove")
         assert_equal(len(self.nodes[1].listbanned()), 1)
 
         self.log.info("setban remove: successfully unban subnet")
@@ -81,10 +81,10 @@ def run_test(self):
         self.log.info("disconnectnode: fail to disconnect when calling with address and nodeid")
         address1 = self.nodes[0].getpeerinfo()[0]['addr']
         node1 = self.nodes[0].getpeerinfo()[0]['addr']
-        assert_raises_jsonrpc(-32602, "Only one of address and nodeid should be provided.", self.nodes[0].disconnectnode, address=address1, nodeid=node1)
+        assert_raises_rpc_error(-32602, "Only one of address and nodeid should be provided.", self.nodes[0].disconnectnode, address=address1, nodeid=node1)
 
         self.log.info("disconnectnode: fail to disconnect when calling with junk address")
-        assert_raises_jsonrpc(-29, "Node not found in connected nodes", self.nodes[0].disconnectnode, address="221B Baker Street")
+        assert_raises_rpc_error(-29, "Node not found in connected nodes", self.nodes[0].disconnectnode, address="221B Baker Street")
 
         self.log.info("disconnectnode: successfully disconnect node by address")
         address1 = self.nodes[0].getpeerinfo()[0]['addr']

test/functional/fundrawtransaction.py

@@ -179,7 +179,7 @@ def run_test(self):
         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
         assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
 
-        assert_raises_jsonrpc(-3, "Unexpected key foo", self.nodes[2].fundrawtransaction, rawtx, {'foo':'bar'})
+        assert_raises_rpc_error(-3, "Unexpected key foo", self.nodes[2].fundrawtransaction, rawtx, {'foo':'bar'})
 
         ############################################################
         # test a fundrawtransaction with an invalid change address #
@@ -192,7 +192,7 @@ def run_test(self):
         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
         assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
 
-        assert_raises_jsonrpc(-5, "changeAddress must be a valid bitcoin address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'})
+        assert_raises_rpc_error(-5, "changeAddress must be a valid bitcoin address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'})
 
         ############################################################
         # test a fundrawtransaction with a provided change address #
@@ -206,7 +206,7 @@ def run_test(self):
         assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
 
         change = self.nodes[2].getnewaddress()
-        assert_raises_jsonrpc(-8, "changePosition out of bounds", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':change, 'changePosition':2})
+        assert_raises_rpc_error(-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]
@@ -314,7 +314,7 @@ def run_test(self):
         rawtx   = self.nodes[2].createrawtransaction(inputs, outputs)
         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
 
-        assert_raises_jsonrpc(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx)
+        assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx)
 
         ############################################################
         #compare fee of a standard pubkeyhash transaction
@@ -469,14 +469,14 @@ 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
-        assert_raises_jsonrpc(-4, "Keypool ran out, please call keypoolrefill first", self.nodes[1].fundrawtransaction, rawtx)
+        assert_raises_rpc_error(-4, "Keypool ran out, please call keypoolrefill first", self.nodes[1].fundrawtransaction, rawtx)
 
         #refill the keypool
         self.nodes[1].walletpassphrase("test", 100)
         self.nodes[1].keypoolrefill(8) #need to refill the keypool to get an internal change address
         self.nodes[1].walletlock()
 
-        assert_raises_jsonrpc(-13, "walletpassphrase", self.nodes[1].sendtoaddress, self.nodes[0].getnewaddress(), 1.2)
+        assert_raises_rpc_error(-13, "walletpassphrase", self.nodes[1].sendtoaddress, self.nodes[0].getnewaddress(), 1.2)
 
         oldBalance = self.nodes[0].getbalance()
 

test/functional/import-rescan.py

@@ -19,9 +19,8 @@
 happened previously.
 """
 
-from test_framework.authproxy import JSONRPCException
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import (connect_nodes, sync_blocks, assert_equal, set_node_times)
+from test_framework.util import (assert_raises_rpc_error, connect_nodes, sync_blocks, assert_equal, set_node_times)
 
 import collections
 import enum
@@ -35,21 +34,26 @@
 class Variant(collections.namedtuple("Variant", "call data rescan prune")):
     """Helper for importing one key and verifying scanned transactions."""
 
+    def try_rpc(self, func, *args, **kwargs):
+        if self.expect_disabled:
+            assert_raises_rpc_error(-4, "Rescan is disabled in pruned mode", func, *args, **kwargs)
+        else:
+            return func(*args, **kwargs)
+
     def do_import(self, timestamp):
         """Call one key import RPC."""
 
         if self.call == Call.single:
             if self.data == Data.address:
-                response, error = try_rpc(self.node.importaddress, self.address["address"], self.label,
-                                          self.rescan == Rescan.yes)
+                response = self.try_rpc(self.node.importaddress, self.address["address"], self.label,
+                                               self.rescan == Rescan.yes)
             elif self.data == Data.pub:
-                response, error = try_rpc(self.node.importpubkey, self.address["pubkey"], self.label,
-                                          self.rescan == Rescan.yes)
+                response = self.try_rpc(self.node.importpubkey, self.address["pubkey"], self.label,
+                                               self.rescan == Rescan.yes)
             elif self.data == Data.priv:
-                response, error = try_rpc(self.node.importprivkey, self.key, self.label, self.rescan == Rescan.yes)
+                response = self.try_rpc(self.node.importprivkey, self.key, self.label, self.rescan == Rescan.yes)
             assert_equal(response, None)
-            assert_equal(error, {'message': 'Rescan is disabled in pruned mode',
-                                 'code': -4} if self.expect_disabled else None)
+
         elif self.call == Call.multi:
             response = self.node.importmulti([{
                 "scriptPubKey": {
@@ -179,13 +183,5 @@ def run_test(self):
             else:
                 variant.check()
 
-
-def try_rpc(func, *args, **kwargs):
-    try:
-        return func(*args, **kwargs), None
-    except JSONRPCException as e:
-        return None, e.error
-
-
 if __name__ == "__main__":
     ImportRescanTest().main()

test/functional/importmulti.py

@@ -423,11 +423,11 @@ def run_test (self):
 
         # Bad or missing timestamps
         self.log.info("Should throw on invalid or missing timestamp values")
-        assert_raises_message(JSONRPCException, 'Missing required timestamp field for key',
+        assert_raises_rpc_error(-3, 'Missing required timestamp field for key',
             self.nodes[1].importmulti, [{
                 "scriptPubKey": address['scriptPubKey'],
             }])
-        assert_raises_message(JSONRPCException, 'Expected number or "now" timestamp value for key. got type string',
+        assert_raises_rpc_error(-3, 'Expected number or "now" timestamp value for key. got type string',
             self.nodes[1].importmulti, [{
                 "scriptPubKey": address['scriptPubKey'],
                 "timestamp": "",

test/functional/importprunedfunds.py

@@ -66,7 +66,7 @@ def run_test(self):
         self.sync_all()
 
         #Import with no affiliated address
-        assert_raises_jsonrpc(-5, "No addresses", self.nodes[1].importprunedfunds, rawtxn1, proof1)
+        assert_raises_rpc_error(-5, "No addresses", self.nodes[1].importprunedfunds, rawtxn1, proof1)
 
         balance1 = self.nodes[1].getbalance("", 0, True)
         assert_equal(balance1, Decimal(0))
@@ -97,7 +97,7 @@ def run_test(self):
         assert_equal(address_info['ismine'], True)
 
         #Remove transactions
-        assert_raises_jsonrpc(-8, "Transaction does not exist in wallet.", self.nodes[1].removeprunedfunds, txnid1)
+        assert_raises_rpc_error(-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'))