Merge #13049: [0.16] qa: Backports

41c29f6d1d qa: Fix python TypeError in script.py (MarcoFalke)
7460945e0b [qa] Delete cookie file before starting node (Suhas Daftuar)
0a76ed232a qa: Cache only chain and wallet for regtest datadir (MarcoFalke)
6c26df06ad [qa] Ensure bitcoind processes are cleaned up when tests end (Suhas Daftuar)
df38b130d9 [tests] Test starting bitcoind with -h and -version (John Newbery)
4bdb0ce517 [tests] Fix intermittent rpc_net.py failure. (John Newbery)
0e98f96e42 test: Use wait_until in tests where time was used for polling (Ben Woosley)
1286f3e49a test: Use wait_until to ensure ping goes out (Ben Woosley)
cfebd400ef [test] Round target fee to 8 decimals in assert_fee_amount (Karl-Johan Alm)

Pull request description:

  Similar to #12967 this contains all relevant bugfixes and improvements to the functional test suite.

  I didn't include fixes to make the tests run on Windows, since that is still an ongoing effort and doesn't seem worth to backport.

  As all of these are clean cherry-picks, I suggest reviewers redo the cherry-picks to get the same branch and then run the extended test suite.

Tree-SHA512: 70e1bc28d5572f93796f1ac4d97d77e8146869c15dcc1e3b65a730fa2641283050f769cefd9791d800c758e0a92f11fd55ed0797ccec87b897c7e701d0187f34

10 files changed, 121 insertions, 65 deletions

diff --git a/test/functional/feature_help.py b/test/functional/feature_help.py
new file mode 100755
index 0000000000..fd4a72f628
--- /dev/null
+++ b/test/functional/feature_help.py
@@ -0,0 +1,46 @@
+#!/usr/bin/env python3
+# Copyright (c) 2018 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Verify that starting bitcoin with -h works as expected."""
+import subprocess
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+class HelpTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def setup_network(self):
+        self.add_nodes(self.num_nodes)
+        # Don't start the node
+
+    def run_test(self):
+        self.log.info("Start bitcoin with -h for help text")
+        self.nodes[0].start(extra_args=['-h'], stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+        # Node should exit immediately and output help to stdout.
+        ret_code = self.nodes[0].process.wait(timeout=1)
+        assert_equal(ret_code, 0)
+        output = self.nodes[0].process.stdout.read()
+        assert b'Options' in output
+        self.log.info("Help text received: {} (...)".format(output[0:60]))
+        self.nodes[0].running = False
+
+        self.log.info("Start bitcoin with -version for version information")
+        self.nodes[0].start(extra_args=['-version'], stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+        # Node should exit immediately and output version to stdout.
+        ret_code = self.nodes[0].process.wait(timeout=1)
+        assert_equal(ret_code, 0)
+        output = self.nodes[0].process.stdout.read()
+        assert b'version' in output
+        self.log.info("Version text received: {} (...)".format(output[0:60]))
+        # Clean up TestNode state
+        self.nodes[0].running = False
+        self.nodes[0].process = None
+        self.nodes[0].rpc_connected = False
+        self.nodes[0].rpc = None
+
+if __name__ == '__main__':
+    HelpTest().main()
diff --git a/test/functional/feature_pruning.py b/test/functional/feature_pruning.py
index 49ad7f838c..cb2dd1f6a8 100755
--- a/test/functional/feature_pruning.py
+++ b/test/functional/feature_pruning.py
@@ -11,7 +11,6 @@ This test takes 30 mins or more (up to 2 hours)
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import *
-import time
 import os
 
 MIN_BLOCKS_TO_KEEP = 288
@@ -79,11 +78,8 @@ class PruneTest(BitcoinTestFramework):
         for i in range(25):
             mine_large_block(self.nodes[0], self.utxo_cache_0)
 
-        waitstart = time.time()
-        while os.path.isfile(self.prunedir+"blk00000.dat"):
-            time.sleep(0.1)
-            if time.time() - waitstart > 30:
-                raise AssertionError("blk00000.dat not pruned when it should be")
+        # Wait for blk00000.dat to be pruned
+        wait_until(lambda: not os.path.isfile(self.prunedir+"blk00000.dat"), timeout=30)
 
         self.log.info("Success")
         usage = calc_usage(self.prunedir)
@@ -218,11 +214,8 @@ class PruneTest(BitcoinTestFramework):
             goalbestheight = first_reorg_height + 1
 
         self.log.info("Verify node 2 reorged back to the main chain, some blocks of which it had to redownload")
-        waitstart = time.time()
-        while self.nodes[2].getblockcount() < goalbestheight:
-            time.sleep(0.1)
-            if time.time() - waitstart > 900:
-                raise AssertionError("Node 2 didn't reorg to proper height")
+        # Wait for Node 2 to reorg to proper height
+        wait_until(lambda: self.nodes[2].getblockcount() >= goalbestheight, timeout=900)
         assert(self.nodes[2].getbestblockhash() == goalbesthash)
         # Verify we can now have the data for a block previously pruned
         assert(self.nodes[2].getblock(self.forkhash)["height"] == self.forkheight)
diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py
index ac67e6e9ba..d1d3f1d7f1 100755
--- a/test/functional/feature_reindex.py
+++ b/test/functional/feature_reindex.py
@@ -10,8 +10,7 @@
 """
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal
-import time
+from test_framework.util import wait_until
 
 class ReindexTest(BitcoinTestFramework):
 
@@ -25,9 +24,7 @@ class ReindexTest(BitcoinTestFramework):
         self.stop_nodes()
         extra_args = [["-reindex-chainstate" if justchainstate else "-reindex", "-checkblockindex=1"]]
         self.start_nodes(extra_args)
-        while self.nodes[0].getblockcount() < blockcount:
-            time.sleep(0.1)
-        assert_equal(self.nodes[0].getblockcount(), blockcount)
+        wait_until(lambda: self.nodes[0].getblockcount() == blockcount)
         self.log.info("Success")
 
     def run_test(self):
diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index 16e4f6adb4..72b5f4748f 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -7,14 +7,14 @@
 Tests correspond to code in rpc/net.cpp.
 """
 
-import time
-
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_greater_than_or_equal,
     assert_raises_rpc_error,
     connect_nodes_bi,
     p2p_port,
+    wait_until,
 )
 
 class NetTest(BitcoinTestFramework):
@@ -34,27 +34,34 @@ class NetTest(BitcoinTestFramework):
         assert_equal(self.nodes[0].getconnectioncount(), 2)
 
     def _test_getnettotals(self):
-        # check that getnettotals totalbytesrecv and totalbytessent
-        # are consistent with getpeerinfo
+        # getnettotals totalbytesrecv and totalbytessent should be
+        # consistent with getpeerinfo. Since the RPC calls are not atomic,
+        # and messages might have been recvd or sent between RPC calls, call
+        # getnettotals before and after and verify that the returned values
+        # from getpeerinfo are bounded by those values.
+        net_totals_before = self.nodes[0].getnettotals()
         peer_info = self.nodes[0].getpeerinfo()
+        net_totals_after = self.nodes[0].getnettotals()
         assert_equal(len(peer_info), 2)
-        net_totals = self.nodes[0].getnettotals()
-        assert_equal(sum([peer['bytesrecv'] for peer in peer_info]),
-                     net_totals['totalbytesrecv'])
-        assert_equal(sum([peer['bytessent'] for peer in peer_info]),
-                     net_totals['totalbytessent'])
+        peers_recv = sum([peer['bytesrecv'] for peer in peer_info])
+        peers_sent = sum([peer['bytessent'] for peer in peer_info])
+
+        assert_greater_than_or_equal(peers_recv, net_totals_before['totalbytesrecv'])
+        assert_greater_than_or_equal(net_totals_after['totalbytesrecv'], peers_recv)
+        assert_greater_than_or_equal(peers_sent, net_totals_before['totalbytessent'])
+        assert_greater_than_or_equal(net_totals_after['totalbytessent'], peers_sent)
+
         # test getnettotals and getpeerinfo by doing a ping
         # the bytes sent/received should change
         # note ping and pong are 32 bytes each
         self.nodes[0].ping()
-        time.sleep(0.1)
+        wait_until(lambda: (self.nodes[0].getnettotals()['totalbytessent'] >= net_totals_after['totalbytessent'] + 32 * 2), timeout=1)
+        wait_until(lambda: (self.nodes[0].getnettotals()['totalbytesrecv'] >= net_totals_after['totalbytesrecv'] + 32 * 2), timeout=1)
+
         peer_info_after_ping = self.nodes[0].getpeerinfo()
-        net_totals_after_ping = self.nodes[0].getnettotals()
         for before, after in zip(peer_info, peer_info_after_ping):
-            assert_equal(before['bytesrecv_per_msg']['pong'] + 32, after['bytesrecv_per_msg']['pong'])
-            assert_equal(before['bytessent_per_msg']['ping'] + 32, after['bytessent_per_msg']['ping'])
-        assert_equal(net_totals['totalbytesrecv'] + 32*2, net_totals_after_ping['totalbytesrecv'])
-        assert_equal(net_totals['totalbytessent'] + 32*2, net_totals_after_ping['totalbytessent'])
+            assert_greater_than_or_equal(after['bytesrecv_per_msg']['pong'], before['bytesrecv_per_msg']['pong'] + 32)
+            assert_greater_than_or_equal(after['bytessent_per_msg']['ping'], before['bytessent_per_msg']['ping'] + 32)
 
     def _test_getnetworkinginfo(self):
         assert_equal(self.nodes[0].getnetworkinfo()['networkactive'], True)
@@ -62,12 +69,8 @@ class NetTest(BitcoinTestFramework):
 
         self.nodes[0].setnetworkactive(False)
         assert_equal(self.nodes[0].getnetworkinfo()['networkactive'], False)
-        timeout = 3
-        while self.nodes[0].getnetworkinfo()['connections'] != 0:
-            # Wait a bit for all sockets to close
-            assert timeout > 0, 'not all connections closed in time'
-            timeout -= 0.1
-            time.sleep(0.1)
+        # Wait a bit for all sockets to close
+        wait_until(lambda: self.nodes[0].getnetworkinfo()['connections'] == 0, timeout=3)
 
         self.nodes[0].setnetworkactive(True)
         connect_nodes_bi(self.nodes, 0, 1)
@@ -84,8 +87,7 @@ class NetTest(BitcoinTestFramework):
         assert_equal(len(added_nodes), 1)
         assert_equal(added_nodes[0]['addednode'], ip_port)
         # check that a non-existent node returns an error
-        assert_raises_rpc_error(-24, "Node has not been added",
-                              self.nodes[0].getaddednodeinfo, '1.1.1.1')
+        assert_raises_rpc_error(-24, "Node has not been added", self.nodes[0].getaddednodeinfo, '1.1.1.1')
 
     def _test_getpeerinfo(self):
         peer_info = [x.getpeerinfo() for x in self.nodes]
diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
index dae8a4e569..6fe0b445da 100644
--- a/test/functional/test_framework/script.py
+++ b/test/functional/test_framework/script.py
@@ -526,11 +526,9 @@ class CScript(bytes):
                     yield CScriptOp(opcode)
 
     def __repr__(self):
-        # For Python3 compatibility add b before strings so testcases don't
-        # need to change
         def _repr(o):
             if isinstance(o, bytes):
-                return b"x('%s')" % hexlify(o).decode('ascii')
+                return "x('%s')" % hexlify(o).decode('ascii')
             else:
                 return repr(o)
 
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index f8d66def64..59ec5117e5 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -24,8 +24,8 @@ from .util import (
     check_json_precision,
     connect_nodes_bi,
     disconnect_nodes,
+    get_datadir_path,
     initialize_datadir,
-    log_filename,
     p2p_port,
     set_node_times,
     sync_blocks,
@@ -145,6 +145,8 @@ class BitcoinTestFramework():
             if self.nodes:
                 self.stop_nodes()
         else:
+            for node in self.nodes:
+                node.cleanup_on_exit = False
             self.log.info("Note: bitcoinds were not stopped and may still be running")
 
         if not self.options.nocleanup and not self.options.noshutdown and success != TestStatus.FAILED:
@@ -372,7 +374,7 @@ class BitcoinTestFramework():
         assert self.num_nodes <= MAX_NODES
         create_cache = False
         for i in range(MAX_NODES):
-            if not os.path.isdir(os.path.join(self.options.cachedir, 'node' + str(i))):
+            if not os.path.isdir(get_datadir_path(self.options.cachedir, i)):
                 create_cache = True
                 break
 
@@ -381,8 +383,8 @@ class BitcoinTestFramework():
 
             # find and delete old cache directories if any exist
             for i in range(MAX_NODES):
-                if os.path.isdir(os.path.join(self.options.cachedir, "node" + str(i))):
-                    shutil.rmtree(os.path.join(self.options.cachedir, "node" + str(i)))
+                if os.path.isdir(get_datadir_path(self.options.cachedir, i)):
+                    shutil.rmtree(get_datadir_path(self.options.cachedir, i))
 
             # Create cache directories, run bitcoinds:
             for i in range(MAX_NODES):
@@ -420,15 +422,18 @@ class BitcoinTestFramework():
             self.stop_nodes()
             self.nodes = []
             self.disable_mocktime()
+
+            def cache_path(n, *paths):
+                return os.path.join(get_datadir_path(self.options.cachedir, n), "regtest", *paths)
+
             for i in range(MAX_NODES):
-                os.remove(log_filename(self.options.cachedir, i, "debug.log"))
-                os.remove(log_filename(self.options.cachedir, i, "wallets/db.log"))
-                os.remove(log_filename(self.options.cachedir, i, "peers.dat"))
-                os.remove(log_filename(self.options.cachedir, i, "fee_estimates.dat"))
+                for entry in os.listdir(cache_path(i)):
+                    if entry not in ['wallets', 'chainstate', 'blocks']:
+                        os.remove(cache_path(i, entry))
 
         for i in range(self.num_nodes):
-            from_dir = os.path.join(self.options.cachedir, "node" + str(i))
-            to_dir = os.path.join(self.options.tmpdir, "node" + str(i))
+            from_dir = get_datadir_path(self.options.cachedir, i)
+            to_dir = get_datadir_path(self.options.tmpdir, i)
             shutil.copytree(from_dir, to_dir)
             initialize_datadir(self.options.tmpdir, i)  # Overwrite port/rpcport in bitcoin.conf
 
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index 1054e6d028..f6e4871299 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -17,6 +17,7 @@ import time
 from .authproxy import JSONRPCException
 from .util import (
     assert_equal,
+    delete_cookie_file,
     get_rpc_proxy,
     rpc_url,
     wait_until,
@@ -70,9 +71,20 @@ class TestNode():
         self.rpc = None
         self.url = None
         self.log = logging.getLogger('TestFramework.node%d' % i)
+        self.cleanup_on_exit = True # Whether to kill the node when this object goes away
 
         self.p2ps = []
 
+    def __del__(self):
+        # Ensure that we don't leave any bitcoind processes lying around after
+        # the test ends
+        if self.process and self.cleanup_on_exit:
+            # Should only happen on test failure
+            # Avoid using logger, as that may have already been shutdown when
+            # this destructor is called.
+            print("Cleaning up leftover process")
+            self.process.kill()
+
     def __getattr__(self, name):
         """Dispatches any unrecognised messages to the RPC connection or a CLI instance."""
         if self.use_cli:
@@ -87,6 +99,10 @@ class TestNode():
             extra_args = self.extra_args
         if stderr is None:
             stderr = self.stderr
+        # Delete any existing cookie file -- if such a file exists (eg due to
+        # unclean shutdown), it will get overwritten anyway by bitcoind, and
+        # potentially interfere with our attempt to authenticate
+        delete_cookie_file(self.datadir)
         self.process = subprocess.Popen(self.args + extra_args, stderr=stderr, *args, **kwargs)
         self.running = True
         self.log.debug("bitcoind started, waiting for RPC to come up")
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 7fdc171332..ead1f844f7 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -26,7 +26,7 @@ logger = logging.getLogger("TestFramework.utils")
 
 def assert_fee_amount(fee, tx_size, fee_per_kB):
     """Assert the fee was in range"""
-    target_fee = tx_size * fee_per_kB / 1000
+    target_fee = round(tx_size * fee_per_kB / 1000, 8)
     if fee < target_fee:
         raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)" % (str(fee), str(target_fee)))
     # allow the wallet's estimation to be at most 2 bytes off
@@ -319,8 +319,11 @@ def get_auth_cookie(datadir):
         raise ValueError("No RPC credentials")
     return user, password
 
-def log_filename(dirname, n_node, logname):
-    return os.path.join(dirname, "node" + str(n_node), "regtest", logname)
+# If a cookie file exists in the given datadir, delete it.
+def delete_cookie_file(datadir):
+    if os.path.isfile(os.path.join(datadir, "regtest", ".cookie")):
+        logger.debug("Deleting leftover cookie file")
+        os.remove(os.path.join(datadir, "regtest", ".cookie"))
 
 def get_bip9_status(node, key):
     info = node.getblockchaininfo()
@@ -334,20 +337,15 @@ def disconnect_nodes(from_connection, node_num):
     for peer_id in [peer['id'] for peer in from_connection.getpeerinfo() if "testnode%d" % node_num in peer['subver']]:
         from_connection.disconnectnode(nodeid=peer_id)
 
-    for _ in range(50):
-        if [peer['id'] for peer in from_connection.getpeerinfo() if "testnode%d" % node_num in peer['subver']] == []:
-            break
-        time.sleep(0.1)
-    else:
-        raise AssertionError("timed out waiting for disconnect")
+    # wait to disconnect
+    wait_until(lambda: [peer['id'] for peer in from_connection.getpeerinfo() if "testnode%d" % node_num in peer['subver']] == [], timeout=5)
 
 def connect_nodes(from_connection, node_num):
     ip_port = "127.0.0.1:" + str(p2p_port(node_num))
     from_connection.addnode(ip_port, "onetry")
     # poll until version handshake complete to avoid race conditions
     # with transaction relaying
-    while any(peer['version'] == 0 for peer in from_connection.getpeerinfo()):
-        time.sleep(0.1)
+    wait_until(lambda:  all(peer['version'] != 0 for peer in from_connection.getpeerinfo()))
 
 def connect_nodes_bi(nodes, a, b):
     connect_nodes(nodes[a], b)
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 98944685e1..fbd155438e 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -133,6 +133,7 @@ BASE_SCRIPTS= [
     'feature_logging.py',
     'p2p_node_network_limited.py',
     'feature_config_args.py',
+    'feature_help.py',
     # Don't append tests at the end to avoid merge conflicts
     # Put them in a random line within the section that fits their approximate run-time
 ]
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index a90dbc8adf..06300d3de6 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -379,9 +379,9 @@ class WalletTest(BitcoinTestFramework):
             self.start_node(0, [m, "-limitancestorcount="+str(chainlimit)])
             self.start_node(1, [m, "-limitancestorcount="+str(chainlimit)])
             self.start_node(2, [m, "-limitancestorcount="+str(chainlimit)])
-            while m == '-reindex' and [block_count] * 3 != [self.nodes[i].getblockcount() for i in range(3)]:
+            if m == '-reindex':
                 # reindex will leave rpc warm up "early"; Wait for it to finish
-                time.sleep(0.1)
+                wait_until(lambda: [block_count] * 3 == [self.nodes[i].getblockcount() for i in range(3)])
             assert_equal(balance_nodes, [self.nodes[i].getbalance() for i in range(3)])
 
         # Exercise listsinceblock with the last two blocks