Merge #17288: Added TestShell class for interactive Python environments.

19139ee034d20ebab1b91d3ac13a8eee70b59374 Add documentation for test_shell submodule (JamesC)
f5112369cf91451d2d0bf574a9bfdaea04696939 Add TestShell class (James Chiang)
5155602a636c323424f75272ccec38588b3d71cd Move argparse() to init() (JamesC)
2ab01462f48b2d4e0d03ba842c3af8851c67c6f1 Move assert num_nodes is set into main() (JamesC)
614c645643e86c4255b98c663c10f2c227158d4b Clear TestNode objects after shutdown (JamesC)
6f40820757d25ff1ccfdfcbdf2b45b8b65308010 Add closing and flushing of logging handlers (JamesC)
6b71241291a184c9ee197bf5f0c7e1414417a0a0 Refactor TestFramework main() into setup/shutdown (JamesC)
ede8b7608e115364b5bb12e7f39d662145733de6 Remove network_event_loop instance in close() (JamesC)

Pull request description:

  This PR refactors BitcoinTestFramework to encapsulate setup and shutdown logic into dedicated methods, and adds a  ~~TestWrapper~~ TestShell child class. This wrapper allows the underlying BitcoinTestFramework to run _between user inputs_ in a REPL environment, such as a Jupyter notebook or any interactive Python3 interpreter.

  The ~~TestWrapper~~ TestShell is motivated by the opportunity to expose the test-framework as a prototyping and educational toolkit. Examples of code prototypes enabled by  ~~TestWrapper~~ TestShell can be found in the Optech [Taproot/Schnorr](https://github.com/bitcoinops/taproot-workshop) workshop repository.

  Usage example:
  ```
  >>> import sys
  >>> sys.path.insert(0, "/path/to/bitcoin/test/functional")
  ```
  ```
  >>> from test_framework.test_wrapper import TestShell
  >>> test = TestShell()
  >>> test.setup(num_nodes=2)
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX
  ```
  ```
  >>> test.nodes[0].generate(101)
  >>> test.nodes[0].getblockchaininfo()["blocks"]
  101
  ```
  ```
  >>> test.shutdown()
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Stopping nodes
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Cleaning up /path/to/bitcoin_func_test_XXXXXXX on exit
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Tests successful
  ```

  **Overview of changes to BitcoinTestFramework:**

  - Code moved to `setup()/shutdown()` methods.
  - Argument parsing logic encapsulated by `parse_args` method.
  - Success state moved to `BitcoinTestFramework.success`.

  _During Shutdown_

  - `BitcoinTestFramework` logging handlers are flushed and removed.
  - `BitcoinTestFrameowork.nodes` list is cleared.
  - `NetworkThread.network_event_loop` is reset. (NetworkThread class).

  **Behavioural changes:**
  - Test parameters can now also be set when overriding BitcoinTestFramework.setup() in addition to overriding `set_test_params` method.
  - Potential exceptions raised in BitcoinTestFramework.setup() will be handled in main().

  **Added files:**
  - ~~test_wrapper.py~~ `test_shell.py`
  - ~~test-wrapper.md~~ `test-shell.md`

ACKs for top commit:
  jamesob:
    ACK https://github.com/bitcoin/bitcoin/pull/17288/commits/19139ee034d20ebab1b91d3ac13a8eee70b59374
  jonatack:
    ACK 19139ee034d20ebab1b91d3ac13a8eee70b59374
  jnewbery:
    Rather than invalidate the three ACKs for a minor nit, can you force push back to 19139ee034d20ebab1b91d3ac13a8eee70b59374 please? I think this PR was ready to merge before your last force push.
  jachiang:
    > Rather than invalidate the three ACKs for a minor nit, can you force push back to [19139ee](https://github.com/bitcoin/bitcoin/commit/19139ee034d20ebab1b91d3ac13a8eee70b59374) please? I think this PR was ready to merge before your last force push.
  jnewbery:
    ACK 19139ee034d20ebab1b91d3ac13a8eee70b59374

Tree-SHA512: 0c24f405f295a8580a9c8f1b9e0182b5d753eb08cc331424616dd50a062fb773d3719db4d08943365b1f42ccb965cc363b4bcc5beae27ac90b3460b349ed46b2

5 files changed, 337 insertions, 33 deletions

diff --git a/test/functional/README.md b/test/functional/README.md
index a9b83076eb..77a9ce9acb 100644
--- a/test/functional/README.md
+++ b/test/functional/README.md
@@ -99,6 +99,16 @@ P2PInterface object and override the callback methods.
 Examples tests are [p2p_unrequested_blocks.py](p2p_unrequested_blocks.py),
 [p2p_compactblocks.py](p2p_compactblocks.py).
 
+#### Prototyping tests
+
+The [`TestShell`](test-shell.md) class exposes the BitcoinTestFramework
+functionality to interactive Python3 environments and can be used to prototype
+tests. This may be especially useful in a REPL environment with session logging
+utilities, such as
+[IPython](https://ipython.readthedocs.io/en/stable/interactive/reference.html#session-logging-and-restoring).
+The logs of such interactive sessions can later be adapted into permanent test
+cases.
+
 ### Test framework modules
 The following are useful modules for test developers. They are located in
 [test/functional/test_framework/](test_framework).
diff --git a/test/functional/test-shell.md b/test/functional/test-shell.md
new file mode 100644
index 0000000000..aefcdc5ec8
--- /dev/null
+++ b/test/functional/test-shell.md
@@ -0,0 +1,188 @@
+Test Shell for Interactive Environments
+=========================================
+
+This document describes how to use the `TestShell` submodule in the functional
+test suite.
+
+The `TestShell` submodule extends the `BitcoinTestFramework` functionality to
+external interactive environments for prototyping and educational purposes. Just
+like `BitcoinTestFramework`, the `TestShell` allows the user to:
+
+* Manage regtest bitcoind subprocesses.
+* Access RPC interfaces of the underlying bitcoind instances.
+* Log events to the functional test logging utility.
+
+The `TestShell` can be useful in interactive environments where it is necessary
+to extend the object lifetime of the underlying `BitcoinTestFramework` between
+user inputs. Such environments include the Python3 command line interpreter or
+[Jupyter](https://jupyter.org/) notebooks running a Python3 kernel.
+
+## 1. Requirements
+
+* Python3
+* `bitcoind` built in the same repository as the `TestShell`.
+
+## 2. Importing `TestShell` from the Bitcoin Core repository
+
+We can import the `TestShell` by adding the path of the Bitcoin Core
+`test_framework` module to the beginning of the PATH variable, and then
+importing the `TestShell` class from the `test_shell` sub-package.
+
+```
+>>> import sys
+>>> sys.path.insert(0, "/path/to/bitcoin/test/functional")
+>>> from test_framework.test_shell import `TestShell`
+```
+
+The following `TestShell` methods manage the lifetime of the underlying bitcoind
+processes and logging utilities.
+
+* `TestShell.setup()`
+* `TestShell.shutdown()`
+
+The `TestShell` inherits all `BitcoinTestFramework` members and methods, such
+as:
+* `TestShell.nodes[index].rpc_method()`
+* `TestShell.log.info("Custom log message")`
+
+The following sections demonstrate how to initialize, run, and shut down a
+`TestShell` object.
+
+## 3. Initializing a `TestShell` object
+
+```
+>>> test = TestShell()
+>>> test.setup(num_nodes=2, setup_clean_chain=True)
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX
+```
+The `TestShell` forwards all functional test parameters of the parent
+`BitcoinTestFramework` object. The full set of argument keywords which can be
+used to initialize the `TestShell` can be found in [section
+#6](#custom-testshell-parameters) of this document.
+
+**Note: Running multiple instances of `TestShell` is not allowed.** Running a
+single process also ensures that logging remains consolidated in the same
+temporary folder. If you need more bitcoind nodes than set by default (1),
+simply increase the `num_nodes` parameter during setup.
+
+```
+>>> test2 = TestShell()
+>>> test2.setup()
+TestShell is already running!
+```
+
+## 4. Interacting with the `TestShell`
+
+Unlike the `BitcoinTestFramework` class, the `TestShell` keeps the underlying
+Bitcoind subprocesses (nodes) and logging utilities running until the user
+explicitly shuts down the `TestShell` object.
+
+During the time between the `setup` and `shutdown` calls, all `bitcoind` node
+processes and `BitcoinTestFramework` convenience methods can be accessed
+interactively.
+
+**Example: Mining a regtest chain**
+
+By default, the `TestShell` nodes are initialized with a clean chain. This means
+that each node of the `TestShell` is initialized with a block height of 0.
+
+```
+>>> test.nodes[0].getblockchaininfo()["blocks"]
+0
+```
+
+We now let the first node generate 101 regtest blocks, and direct the coinbase
+rewards to a wallet address owned by the mining node.
+
+```
+>>> address = test.nodes[0].getnewaddress()
+>>> test.nodes[0].generatetoaddress(101, address)
+['2b98dd0044aae6f1cca7f88a0acf366a4bfe053c7f7b00da3c0d115f03d67efb', ...
+```
+Since the two nodes are both initialized by default to establish an outbound
+connection to each other during `setup`, the second node's chain will include
+the mined blocks as soon as they propagate.
+
+```
+>>> test.nodes[1].getblockchaininfo()["blocks"]
+101
+```
+The block rewards from the first block are now spendable by the wallet of the
+first node.
+
+```
+>>> test.nodes[0].getbalance()
+Decimal('50.00000000')
+```
+
+We can also log custom events to the logger.
+
+```
+>>> test.nodes[0].log.info("Successfully mined regtest chain!")
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework.node0 (INFO): Successfully mined regtest chain!
+```
+
+**Note: Please also consider the functional test
+[readme](../test/functional/README.md), which provides an overview of the
+test-framework**. Modules such as
+[key.py](../test/functional/test_framework/key.py),
+[script.py](../test/functional/test_framework/script.py) and
+[messages.py](../test/functional/test_framework/messages.py) are particularly
+useful in constructing objects which can be passed to the bitcoind nodes managed
+by a running `TestShell` object.
+
+## 5. Shutting the `TestShell` down
+
+Shutting down the `TestShell` will safely tear down all running bitcoind
+instances and remove all temporary data and logging directories.
+
+```
+>>> test.shutdown()
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Stopping nodes
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Cleaning up /path/to/bitcoin_func_test_XXXXXXX on exit
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Tests successful
+```
+To prevent the logs from being removed after a shutdown, simply set the
+`TestShell.options.nocleanup` member to `True`.
+```
+>>> test.options.nocleanup = True
+>>> test.shutdown()
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Stopping nodes
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Not cleaning up dir /path/to/bitcoin_func_test_XXXXXXX on exit
+20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Tests successful
+```
+
+The following utility consolidates logs from the bitcoind nodes and the
+underlying `BitcoinTestFramework`:
+
+* `/path/to/bitcoin/test/functional/combine_logs.py
+  '/path/to/bitcoin_func_test_XXXXXXX'`
+
+## 6. Custom `TestShell` parameters
+
+The `TestShell` object initializes with the default settings inherited from the
+`BitcoinTestFramework` class. The user can override these in
+`TestShell.setup(key=value)`.
+
+**Note:** `TestShell.reset()` will reset test parameters to default values and
+can be called after the TestShell is shut down.
+
+| Test parameter key | Default Value | Description |
+|---|---|---|
+| `bind_to_localhost_only` | `True` | Binds bitcoind RPC services to `127.0.0.1` if set to `True`.|
+| `cachedir` | `"/path/to/bitcoin/test/cache"` | Sets the bitcoind datadir directory. |
+| `chain`  | `"regtest"` | Sets the chain-type for the underlying test bitcoind processes. |
+| `configfile` | `"/path/to/bitcoin/test/config.ini"` | Sets the location of the test framework config file. |
+| `coveragedir` | `None` | Records bitcoind RPC test coverage into this directory if set. |
+| `loglevel` | `INFO` | Logs events at this level and higher. Can be set to `DEBUG`, `INFO`, `WARNING`, `ERROR` or `CRITICAL`. |
+| `nocleanup` | `False` | Cleans up temporary test directory if set to `True` during `shutdown`. |
+| `noshutdown` | `False` | Does not stop bitcoind instances after `shutdown` if set to `True`. |
+| `num_nodes` | `1` | Sets the number of initialized bitcoind processes. |
+| `perf` | False | Profiles running nodes with `perf` for the duration of the test if set to `True`. |
+| `rpc_timeout` | `60` | Sets the RPC server timeout for the underlying bitcoind processes. |
+| `setup_clean_chain` | `False` | Initializes an empty blockchain by default. A 199-block-long chain is initialized if set to `True`. |
+| `randomseed` | Random Integer | `TestShell.options.randomseed` is a member of `TestShell` which can be accessed during a test to seed a random generator. User can override default with a constant value for reproducible test runs. |
+| `supports_cli` | `False` | Whether the bitcoin-cli utility is compiled and available for the test. |
+| `tmpdir` | `"/var/folders/.../"` | Sets directory for test logs. Will be deleted upon a successful test run unless `nocleanup` is set to `True` |
+| `trace_rpc` | `False` | Logs all RPC calls if set to `True`. |
+| `usecli` | `False` | Uses the bitcoin-cli interface for all bitcoind commands instead of directly calling the RPC server. Requires `supports_cli`. |
diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py
index f95c158a68..a9e669fea9 100755
--- a/test/functional/test_framework/mininode.py
+++ b/test/functional/test_framework/mininode.py
@@ -478,7 +478,8 @@ class NetworkThread(threading.Thread):
         wait_until(lambda: not self.network_event_loop.is_running(), timeout=timeout)
         self.network_event_loop.close()
         self.join(timeout)
-
+        # Safe to remove event loop.
+        NetworkThread.network_event_loop = None
 
 class P2PDataStore(P2PInterface):
     """A P2P data store class.
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 780aa5fe03..c56c0d06ff 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -99,12 +99,39 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         self.supports_cli = False
         self.bind_to_localhost_only = True
         self.set_test_params()
-
-        assert hasattr(self, "num_nodes"), "Test must set self.num_nodes in set_test_params()"
+        self.parse_args()
 
     def main(self):
         """Main function. This should not be overridden by the subclass test scripts."""
 
+        assert hasattr(self, "num_nodes"), "Test must set self.num_nodes in set_test_params()"
+
+        try:
+            self.setup()
+            self.run_test()
+        except JSONRPCException:
+            self.log.exception("JSONRPC error")
+            self.success = TestStatus.FAILED
+        except SkipTest as e:
+            self.log.warning("Test Skipped: %s" % e.message)
+            self.success = TestStatus.SKIPPED
+        except AssertionError:
+            self.log.exception("Assertion failed")
+            self.success = TestStatus.FAILED
+        except KeyError:
+            self.log.exception("Key error")
+            self.success = TestStatus.FAILED
+        except Exception:
+            self.log.exception("Unexpected exception caught during testing")
+            self.success = TestStatus.FAILED
+        except KeyboardInterrupt:
+            self.log.warning("Exiting after keyboard interrupt")
+            self.success = TestStatus.FAILED
+        finally:
+            exit_code = self.shutdown()
+            sys.exit(exit_code)
+
+    def parse_args(self):
         parser = argparse.ArgumentParser(usage="%(prog)s [options]")
         parser.add_argument("--nocleanup", dest="nocleanup", default=False, action="store_true",
                             help="Leave bitcoinds and test.* datadir on exit or error")
@@ -135,6 +162,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         self.add_options(parser)
         self.options = parser.parse_args()
 
+    def setup(self):
+        """Call this method to start up the test framework object with options set."""
+
         PortSeed.n = self.options.port_seed
 
         check_json_precision()
@@ -181,33 +211,20 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         self.network_thread = NetworkThread()
         self.network_thread.start()
 
-        success = TestStatus.FAILED
+        if self.options.usecli:
+            if not self.supports_cli:
+                raise SkipTest("--usecli specified but test does not support using CLI")
+            self.skip_if_no_cli()
+        self.skip_test_if_missing_module()
+        self.setup_chain()
+        self.setup_network()
 
-        try:
-            if self.options.usecli:
-                if not self.supports_cli:
-                    raise SkipTest("--usecli specified but test does not support using CLI")
-                self.skip_if_no_cli()
-            self.skip_test_if_missing_module()
-            self.setup_chain()
-            self.setup_network()
-            self.run_test()
-            success = TestStatus.PASSED
-        except JSONRPCException:
-            self.log.exception("JSONRPC error")
-        except SkipTest as e:
-            self.log.warning("Test Skipped: %s" % e.message)
-            success = TestStatus.SKIPPED
-        except AssertionError:
-            self.log.exception("Assertion failed")
-        except KeyError:
-            self.log.exception("Key error")
-        except Exception:
-            self.log.exception("Unexpected exception caught during testing")
-        except KeyboardInterrupt:
-            self.log.warning("Exiting after keyboard interrupt")
+        self.success = TestStatus.PASSED
 
-        if success == TestStatus.FAILED and self.options.pdbonfailure:
+    def shutdown(self):
+        """Call this method to shut down the test framework object."""
+
+        if self.success == TestStatus.FAILED and self.options.pdbonfailure:
             print("Testcase failed. Attaching python debugger. Enter ? for help")
             pdb.set_trace()
 
@@ -225,7 +242,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         should_clean_up = (
             not self.options.nocleanup and
             not self.options.noshutdown and
-            success != TestStatus.FAILED and
+            self.success != TestStatus.FAILED and
             not self.options.perf
         )
         if should_clean_up:
@@ -238,20 +255,33 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             self.log.warning("Not cleaning up dir {}".format(self.options.tmpdir))
             cleanup_tree_on_exit = False
 
-        if success == TestStatus.PASSED:
+        if self.success == TestStatus.PASSED:
             self.log.info("Tests successful")
             exit_code = TEST_EXIT_PASSED
-        elif success == TestStatus.SKIPPED:
+        elif self.success == TestStatus.SKIPPED:
             self.log.info("Test skipped")
             exit_code = TEST_EXIT_SKIPPED
         else:
             self.log.error("Test failed. Test logging available at %s/test_framework.log", self.options.tmpdir)
             self.log.error("Hint: Call {} '{}' to consolidate all logs".format(os.path.normpath(os.path.dirname(os.path.realpath(__file__)) + "/../combine_logs.py"), self.options.tmpdir))
             exit_code = TEST_EXIT_FAILED
-        logging.shutdown()
+        # Logging.shutdown will not remove stream- and filehandlers, so we must
+        # do it explicitly. Handlers are removed so the next test run can apply
+        # different log handler settings.
+        # See: https://docs.python.org/3/library/logging.html#logging.shutdown
+        for h in list(self.log.handlers):
+            h.flush()
+            h.close()
+            self.log.removeHandler(h)
+        rpc_logger = logging.getLogger("BitcoinRPC")
+        for h in list(rpc_logger.handlers):
+            h.flush()
+            rpc_logger.removeHandler(h)
         if cleanup_tree_on_exit:
             shutil.rmtree(self.options.tmpdir)
-        sys.exit(exit_code)
+
+        self.nodes.clear()
+        return exit_code
 
     # Methods to override in subclass test scripts.
     def set_test_params(self):
diff --git a/test/functional/test_framework/test_shell.py b/test/functional/test_framework/test_shell.py
new file mode 100644
index 0000000000..79da35b364
--- /dev/null
+++ b/test/functional/test_framework/test_shell.py
@@ -0,0 +1,75 @@
+#!/usr/bin/env python3
+# Copyright (c) 2019 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.test_framework import BitcoinTestFramework
+
+class TestShell:
+    """Wrapper Class for BitcoinTestFramework.
+
+    The TestShell class extends the BitcoinTestFramework
+    rpc & daemon process management functionality to external
+    python environments.
+
+    It is a singleton class, which ensures that users only
+    start a single TestShell at a time."""
+
+    class __TestShell(BitcoinTestFramework):
+        def set_test_params(self):
+            pass
+
+        def run_test(self):
+            pass
+
+        def setup(self, **kwargs):
+            if self.running:
+                print("TestShell is already running!")
+                return
+
+            # Num_nodes parameter must be set
+            # by BitcoinTestFramework child class.
+            self.num_nodes = kwargs.get('num_nodes', 1)
+            kwargs.pop('num_nodes', None)
+
+            # User parameters override default values.
+            for key, value in kwargs.items():
+                if hasattr(self, key):
+                    setattr(self, key, value)
+                elif hasattr(self.options, key):
+                    setattr(self.options, key, value)
+                else:
+                    raise KeyError(key + " not a valid parameter key!")
+
+            super().setup()
+            self.running = True
+
+        def shutdown(self):
+            if not self.running:
+                print("TestShell is not running!")
+            else:
+                super().shutdown()
+                self.running = False
+
+        def reset(self):
+            if self.running:
+                print("Shutdown TestWrapper before resetting!")
+            else:
+                self.num_nodes = None
+                super().__init__()
+
+    instance = None
+
+    def __new__(cls):
+        # This implementation enforces singleton pattern, and will return the
+        # previously initialized instance if available
+        if not TestShell.instance:
+            TestShell.instance = TestShell.__TestShell()
+            TestShell.instance.running = False
+        return TestShell.instance
+
+    def __getattr__(self, name):
+        return getattr(self.instance, name)
+
+    def __setattr__(self, name, value):
+        return setattr(self.instance, name, value)