pyln-testing: use files for stdout and stderr, not threads.

Some flakes are caused by weird races in this code.  Plus, if we
get things to write straight to files, we might see things in
there on post-mortem which happen after the python runner exits.

It's a bit less efficient, but much simpler.  Let's see if it helps!

Some tests need a rework now, since we don't get a failure (except
eventual timeout), but they're simpler.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

contrib/pyln-testing/pyln/testing/utils.py

@@ -174,13 +174,20 @@ class TailableProc(object):
     tail the processes and react to their output.
     """
 
-    def __init__(self, outputDir=None, verbose=True):
+    def __init__(self, outputDir, verbose=True):
         self.logs = []
-        self.logs_cond = threading.Condition(threading.RLock())
         self.env = os.environ.copy()
-        self.running = False
         self.proc = None
         self.outputDir = outputDir
+        if not os.path.exists(outputDir):
+            os.makedirs(outputDir)
+        # Create and open them.
+        self.stdout_filename = os.path.join(outputDir, "log")
+        self.stderr_filename = os.path.join(outputDir, "errlog")
+        self.stdout_write = open(self.stdout_filename, "wt")
+        self.stderr_write = open(self.stderr_filename, "wt")
+        self.stdout_read = open(self.stdout_filename, "rt")
+        self.stderr_read = open(self.stderr_filename, "rt")
         self.logsearch_start = 0
         self.err_logs = []
         self.prefix = ""
@@ -192,29 +199,17 @@ class TailableProc(object):
         # pass it to the log matcher and not print it to stdout).
         self.log_filter = lambda line: False
 
-    def start(self, stdin=None, stdout=None, stderr=None):
+    def start(self, stdin=None):
         """Start the underlying process and start monitoring it.
         """
         logging.debug("Starting '%s'", " ".join(self.cmd_line))
         self.proc = subprocess.Popen(self.cmd_line,
                                      stdin=stdin,
-                                     stdout=stdout if stdout else subprocess.PIPE,
-                                     stderr=stderr,
+                                     stdout=self.stdout_write,
+                                     stderr=self.stderr_write,
                                      env=self.env)
-        self.thread = threading.Thread(target=self.tail)
-        self.thread.daemon = True
-        self.thread.start()
-        self.running = True
-
-    def save_log(self):
-        if self.outputDir:
-            logpath = os.path.join(self.outputDir, 'log')
-            with open(logpath, 'w') as f:
-                for l in self.logs:
-                    f.write(l + '\n')
 
     def stop(self, timeout=10):
-        self.save_log()
         self.proc.terminate()
 
         # Now give it some time to react to the signal
@@ -224,56 +219,32 @@ class TailableProc(object):
             self.proc.kill()
 
         self.proc.wait()
-        self.thread.join()
-
         return self.proc.returncode
 
     def kill(self):
         """Kill process without giving it warning."""
         self.proc.kill()
         self.proc.wait()
-        self.thread.join()
 
-    def tail(self):
-        """Tail the stdout of the process and remember it.
-
-        Stores the lines of output produced by the process in
-        self.logs and signals that a new line was read so that it can
-        be picked up by consumers.
+    def logs_catchup(self):
+        """Save the latest stdout / stderr contents; return true if we got anything.
         """
-        for line in iter(self.proc.stdout.readline, ''):
-            if len(line) == 0:
-                break
-
-            line = line.decode('UTF-8', 'replace').rstrip()
-
-            if self.log_filter(line):
-                continue
-
-            if self.verbose:
-                sys.stdout.write("{}: {}\n".format(self.prefix, line))
-
-            with self.logs_cond:
-                self.logs.append(line)
-                self.logs_cond.notifyAll()
-
-        self.running = False
-        self.proc.stdout.close()
-
-        if self.proc.stderr:
-            for line in iter(self.proc.stderr.readline, ''):
-
-                if line is None or len(line) == 0:
-                    break
-
-                line = line.rstrip().decode('UTF-8', 'replace')
-                self.err_logs.append(line)
-
-            self.proc.stderr.close()
+        new_stdout = self.stdout_read.readlines()
+        if self.verbose:
+            for line in new_stdout:
+                sys.stdout.write("{}: {}".format(self.prefix, line))
+        self.logs += [l.rstrip() for l in new_stdout]
+        new_stderr = self.stderr_read.readlines()
+        if self.verbose:
+            for line in new_stderr:
+                sys.stderr.write("{}-stderr: {}".format(self.prefix, line))
+        self.err_logs += [l.rstrip() for l in new_stderr]
+        return len(new_stdout) > 0 or len(new_stderr) > 0
 
     def is_in_log(self, regex, start=0):
         """Look for `regex` in the logs."""
 
+        self.logs_catchup()
         ex = re.compile(regex)
         for l in self.logs[start:]:
             if ex.search(l):
@@ -286,6 +257,7 @@ class TailableProc(object):
     def is_in_stderr(self, regex):
         """Look for `regex` in stderr."""
 
+        self.logs_catchup()
         ex = re.compile(regex)
         for l in self.err_logs:
             if ex.search(l):
@@ -311,31 +283,29 @@ class TailableProc(object):
         logging.debug("Waiting for {} in the logs".format(regexs))
         exs = [re.compile(r) for r in regexs]
         start_time = time.time()
-        pos = self.logsearch_start
         while True:
-            if timeout is not None and time.time() > start_time + timeout:
-                print("Time-out: can't find {} in logs".format(exs))
-                for r in exs:
-                    if self.is_in_log(r):
-                        print("({} was previously in logs!)".format(r))
-                raise TimeoutError('Unable to find "{}" in logs.'.format(exs))
+            if self.logsearch_start >= len(self.logs):
+                if not self.logs_catchup():
+                    time.sleep(0.25)
 
-            with self.logs_cond:
-                if pos >= len(self.logs):
-                    if not self.running:
-                        raise ValueError('Process died while waiting for logs')
-                    self.logs_cond.wait(1)
-                    continue
+                if timeout is not None and time.time() > start_time + timeout:
+                    print("Time-out: can't find {} in logs".format(exs))
+                    for r in exs:
+                        if self.is_in_log(r):
+                            print("({} was previously in logs!)".format(r))
+                    raise TimeoutError('Unable to find "{}" in logs.'.format(exs))
+                continue
 
-                for r in exs.copy():
-                    self.logsearch_start = pos + 1
-                    if r.search(self.logs[pos]):
-                        logging.debug("Found '%s' in logs", r)
-                        exs.remove(r)
-                        break
-                if len(exs) == 0:
-                    return self.logs[pos]
-                pos += 1
+            line = self.logs[self.logsearch_start]
+            self.logsearch_start += 1
+            for r in exs.copy():
+                if r.search(line):
+                    logging.debug("Found '%s' in logs", r)
+                    exs.remove(r)
+                    if len(exs) == 0:
+                        return line
+                    # Don't match same line with different regexs!
+                    break
 
     def wait_for_log(self, regex, timeout=TIMEOUT):
         """Look for `regex` in the logs.
@@ -620,10 +590,9 @@ class LightningD(TailableProc):
 
         return self.cmd_prefix + [self.executable] + opts
 
-    def start(self, stdin=None, stdout=None, stderr=None,
-              wait_for_initialized=True):
+    def start(self, stdin=None, wait_for_initialized=True):
         self.opts['bitcoin-rpcport'] = self.rpcproxy.rpcport
-        TailableProc.start(self, stdin, stdout, stderr)
+        TailableProc.start(self, stdin)
         if wait_for_initialized:
             self.wait_for_log("Server started with public key")
         logging.info("LightningD started")
@@ -852,8 +821,8 @@ class LightningNode(object):
             info = self.rpc.getinfo()
         return 'warning_bitcoind_sync' not in info and 'warning_lightningd_sync' not in info
 
-    def start(self, wait_for_bitcoind_sync=True, stderr=None):
-        self.daemon.start(stderr=stderr)
+    def start(self, wait_for_bitcoind_sync=True):
+        self.daemon.start()
         # Cache `getinfo`, we'll be using it a lot
         self.info = self.rpc.getinfo()
         # This shortcut is sufficient for our simple tests.
@@ -878,7 +847,6 @@ class LightningNode(object):
         if self.rc is None:
             self.rc = self.daemon.stop()
 
-        self.daemon.save_log()
         self.daemon.cleanup()
 
         if self.rc != 0 and not self.may_fail:
@@ -1417,12 +1385,7 @@ class NodeFactory(object):
 
         if start:
             try:
-                # Capture stderr if we're failing
-                if expect_fail:
-                    stderr = subprocess.PIPE
-                else:
-                    stderr = None
-                node.start(wait_for_bitcoind_sync, stderr=stderr)
+                node.start(wait_for_bitcoind_sync)
             except Exception:
                 if expect_fail:
                     return node

tests/test_misc.py

@@ -109,13 +109,13 @@ def test_bitcoin_failure(node_factory, bitcoind):
     # Ignore BROKEN log message about blocksonly mode.
     l2 = node_factory.get_node(start=False, expect_fail=True,
                                allow_broken_log=True)
-    with pytest.raises(ValueError):
-        l2.start(stderr=subprocess.PIPE)
+    l2.daemon.start(wait_for_initialized=False)
+    # Will exit with failure code.
+    assert l2.daemon.wait() == 1
     assert l2.daemon.is_in_stderr(r".*deactivating transaction relay is not"
-                                  " supported.") is not None
-    # wait_for_log gets upset since daemon is not running.
-    wait_for(lambda: l2.daemon.is_in_log('deactivating transaction'
-                                         ' relay is not supported'))
+                                  " supported.")
+    assert l2.daemon.is_in_log('deactivating transaction'
+                               ' relay is not supported')
 
 
 def test_bitcoin_ibd(node_factory, bitcoind):
@@ -1207,8 +1207,10 @@ def test_rescan(node_factory, bitcoind):
     l1.daemon.opts['rescan'] = -500000
     l1.stop()
     bitcoind.generate_block(4)
-    with pytest.raises(ValueError):
-        l1.start()
+    l1.daemon.start(wait_for_initialized=False)
+    # Will exit with failure code.
+    assert l1.daemon.wait() == 1
+    assert l1.daemon.is_in_stderr(r"bitcoind has gone backwards from 500000 to 105 blocks!")
 
     # Restarting with future absolute blockheight is fine if we can find it.
     l1.daemon.opts['rescan'] = -105
@@ -1236,14 +1238,19 @@ def test_bitcoind_goes_backwards(node_factory, bitcoind):
     bitcoind.start()
 
     # Will simply refuse to start.
-    with pytest.raises(ValueError):
-        l1.start()
+    l1.daemon.start(wait_for_initialized=False)
+    # Will exit with failure code.
+    assert l1.daemon.wait() == 1
+    assert l1.daemon.is_in_stderr('bitcoind has gone backwards')
 
     # Nor will it start with if we ask for a reindex of fewer blocks.
     l1.daemon.opts['rescan'] = 3
 
-    with pytest.raises(ValueError):
-        l1.start()
+    # Will simply refuse to start.
+    l1.daemon.start(wait_for_initialized=False)
+    # Will exit with failure code.
+    assert l1.daemon.wait() == 1
+    assert l1.daemon.is_in_stderr('bitcoind has gone backwards')
 
     # This will force it, however.
     l1.daemon.opts['rescan'] = -100
@@ -1690,7 +1697,7 @@ def test_newaddr(node_factory, chainparams):
     assert both['bech32'].startswith(chainparams['bip173_prefix'])
 
 
-def test_bitcoind_fail_first(node_factory, bitcoind, executor):
+def test_bitcoind_fail_first(node_factory, bitcoind):
     """Make sure we handle spurious bitcoin-cli failures during startup
 
     See [#2687](https://github.com/ElementsProject/lightning/issues/2687) for
@@ -1699,7 +1706,9 @@ def test_bitcoind_fail_first(node_factory, bitcoind, executor):
     """
     # Do not start the lightning node since we need to instrument bitcoind
     # first.
-    l1 = node_factory.get_node(start=False)
+    l1 = node_factory.get_node(start=False,
+                               allow_broken_log=True,
+                               may_fail=True)
 
     # Instrument bitcoind to fail some queries first.
     def mock_fail(*args):
@@ -1708,22 +1717,17 @@ def test_bitcoind_fail_first(node_factory, bitcoind, executor):
     l1.daemon.rpcproxy.mock_rpc('getblockhash', mock_fail)
     l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', mock_fail)
 
-    f = executor.submit(l1.start)
-
-    wait_for(lambda: l1.daemon.running)
-    # Make sure it fails on the first `getblock` call (need to use `is_in_log`
-    # since the `wait_for_log` in `start` sets the offset)
-    wait_for(lambda: l1.daemon.is_in_log(
-        r'getblockhash [a-z0-9]* exited with status 1'))
-    wait_for(lambda: l1.daemon.is_in_log(
-        r'Unable to estimate opening fees'))
+    l1.daemon.start(wait_for_initialized=False)
+    l1.daemon.wait_for_logs([r'getblockhash [a-z0-9]* exited with status 1',
+                             r'Unable to estimate opening fees',
+                             r'BROKEN.*we have been retrying command for --bitcoin-retry-timeout=60 seconds'])
+    # Will exit with failure code.
+    assert l1.daemon.wait() == 1
 
     # Now unset the mock, so calls go through again
     l1.daemon.rpcproxy.mock_rpc('getblockhash', None)
     l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', None)
 
-    f.result()
-
 
 @pytest.mark.developer("needs --dev-force-bip32-seed")
 @unittest.skipIf(TEST_NETWORK != 'regtest', "Addresses are network specific")
@@ -2077,8 +2081,9 @@ def test_new_node_is_mainnet(node_factory):
     del l1.daemon.opts['network']
 
     # Wrong chain, will fail to start, but that's OK.
-    with pytest.raises(ValueError):
-        l1.start()
+    l1.daemon.start(wait_for_initialized=False)
+    # Will exit with failure code.
+    assert l1.daemon.wait() == 1
 
     # Should create these
     assert os.path.isfile(os.path.join(netdir, "hsm_secret"))

tests/test_plugin.py

@@ -94,7 +94,7 @@ def test_option_types(node_factory):
     }, expect_fail=True, may_fail=True)
 
     # the node should fail to start, and we get a stderr msg
-    assert not n.daemon.running
+    assert n.daemon.wait() == 1
     wait_for(lambda: n.daemon.is_in_stderr('bool_opt: ! does not parse as type bool'))
 
     # What happens if we give it a bad int-option?
@@ -106,7 +106,7 @@ def test_option_types(node_factory):
     }, may_fail=True, expect_fail=True)
 
     # the node should fail to start, and we get a stderr msg
-    assert not n.daemon.running
+    assert n.daemon.wait() == 1
     assert n.daemon.is_in_stderr('--int_opt: notok does not parse as type int')
 
     # Flag opts shouldn't allow any input
@@ -119,7 +119,7 @@ def test_option_types(node_factory):
     }, may_fail=True, expect_fail=True)
 
     # the node should fail to start, and we get a stderr msg
-    assert not n.daemon.running
+    assert n.daemon.wait() == 1
     assert n.daemon.is_in_stderr("--flag_opt: doesn't allow an argument")
 
     n = node_factory.get_node(options={
@@ -1500,9 +1500,10 @@ def test_libplugin(node_factory):
     l1.stop()
     l1.daemon.opts["name-deprecated"] = "test_opt"
 
-    # This actually dies while waiting for the logs.
-    with pytest.raises(ValueError):
-        l1.start()
+    l1.daemon.start(wait_for_initialized=False)
+    # Will exit with failure code.
+    assert l1.daemon.wait() == 1
+    assert l1.daemon.is_in_stderr(r"name-deprecated: deprecated option")
 
     del l1.daemon.opts["name-deprecated"]
     l1.start()
@@ -1646,24 +1647,20 @@ def test_bitcoin_backend(node_factory, bitcoind):
     # We don't start if we haven't all the required methods registered.
     plugin = os.path.join(os.getcwd(), "tests/plugins/bitcoin/part1.py")
     l1.daemon.opts["plugin"] = plugin
-    try:
-        l1.daemon.start()
-    except ValueError:
-        assert l1.daemon.is_in_log("Missing a Bitcoin plugin command")
-        # Now we should start if all the commands are registered, even if they
-        # are registered by two distincts plugins.
-        del l1.daemon.opts["plugin"]
-        l1.daemon.opts["plugin-dir"] = os.path.join(os.getcwd(),
-                                                    "tests/plugins/bitcoin/")
-        try:
-            l1.daemon.start()
-        except ValueError:
-            msg = "All Bitcoin plugin commands registered"
-            assert l1.daemon.is_in_log(msg)
-        else:
-            raise Exception("We registered all commands but couldn't start!")
-    else:
-        raise Exception("We could start without all commands registered !!")
+    l1.daemon.start(wait_for_initialized=False)
+    l1.daemon.wait_for_log("Missing a Bitcoin plugin command")
+    # Will exit with failure code.
+    assert l1.daemon.wait() == 1
+    assert l1.daemon.is_in_stderr(r"Could not access the plugin for sendrawtransaction")
+    # Now we should start if all the commands are registered, even if they
+    # are registered by two distincts plugins.
+    del l1.daemon.opts["plugin"]
+    l1.daemon.opts["plugin-dir"] = os.path.join(os.getcwd(),
+                                                "tests/plugins/bitcoin/")
+    # (it fails when it tries to use them, so startup fails)
+    l1.daemon.start(wait_for_initialized=False)
+    l1.daemon.wait_for_log("All Bitcoin plugin commands registered")
+    assert l1.daemon.wait() == 1
 
     # But restarting with just bcli is ok
     del l1.daemon.opts["plugin-dir"]
@@ -2074,73 +2071,55 @@ def test_important_plugin(node_factory):
     n = node_factory.get_node(options={"important-plugin": os.path.join(pluginsdir, "nonexistent")},
                               may_fail=True, expect_fail=True,
                               allow_broken_log=True, start=False)
-    n.daemon.start(wait_for_initialized=False, stderr=subprocess.PIPE)
-    wait_for(lambda: not n.daemon.running)
-
+    n.daemon.start(wait_for_initialized=False)
+    # Will exit with failure code.
+    assert n.daemon.wait() == 1
     assert n.daemon.is_in_stderr(r"error starting plugin '.*nonexistent'")
 
-    # We use a log file, since our wait_for_log is unreliable when the
-    # daemon actually dies.
-    def get_logfile_match(logpath, regex):
-        if not os.path.exists(logpath):
-            return None
-        with open(logpath, 'r') as f:
-            for line in f.readlines():
-                m = re.search(regex, line)
-                if m is not None:
-                    return m
-        return None
-
-    logpath = os.path.join(n.daemon.lightning_dir, TEST_NETWORK, 'logfile')
-    n.daemon.opts['log-file'] = 'logfile'
-
     # Check we exit if the important plugin dies.
     n.daemon.opts['important-plugin'] = os.path.join(pluginsdir, "fail_by_itself.py")
 
     n.daemon.start(wait_for_initialized=False)
-    wait_for(lambda: not n.daemon.running)
-
-    assert get_logfile_match(logpath,
-                             r'fail_by_itself.py: Plugin marked as important, shutting down lightningd')
-    os.remove(logpath)
+    # Will exit with failure code.
+    assert n.daemon.wait() == 1
+    n.daemon.wait_for_log(r'fail_by_itself.py: Plugin marked as important, shutting down lightningd')
 
     # Check if the important plugin is disabled, we run as normal.
     n.daemon.opts['disable-plugin'] = "fail_by_itself.py"
-    del n.daemon.opts['log-file']
     n.daemon.start()
     # Make sure we can call into a plugin RPC (this is from `bcli`) even
     # if fail_by_itself.py is disabled.
     n.rpc.call("estimatefees", {})
-    # Make sure we are still running.
-    assert n.daemon.running
     n.stop()
 
     # Check if an important plugin dies later, we fail.
     del n.daemon.opts['disable-plugin']
-    n.daemon.opts['log-file'] = 'logfile'
     n.daemon.opts['important-plugin'] = os.path.join(pluginsdir, "suicidal_plugin.py")
 
-    n.daemon.start(wait_for_initialized=False)
-    wait_for(lambda: get_logfile_match(logpath, "Server started with public key"))
+    n.start()
 
     with pytest.raises(RpcError):
         n.rpc.call("die", {})
 
-    wait_for(lambda: not n.daemon.running)
-    assert get_logfile_match(logpath, 'suicidal_plugin.py: Plugin marked as important, shutting down lightningd')
-    os.remove(logpath)
+    # Should exit with exitcode 1
+    n.daemon.wait_for_log('suicidal_plugin.py: Plugin marked as important, shutting down lightningd')
+    assert n.daemon.wait() == 1
+    n.stop()
 
     # Check that if a builtin plugin dies, we fail.
-    n.daemon.start(wait_for_initialized=False)
-
-    wait_for(lambda: get_logfile_match(logpath, r'.*started\(([0-9]*)\).*plugins/pay'))
-    pidstr = get_logfile_match(logpath, r'.*started\(([0-9]*)\).*plugins/pay').group(1)
+    start = n.daemon.logsearch_start
+    n.start()
+    # Reset logsearch_start, since this will predate message that start() looks for.
+    n.daemon.logsearch_start = start
+    line = n.daemon.wait_for_log(r'.*started\([0-9]*\).*plugins/pay')
+    pidstr = re.search(r'.*started\(([0-9]*)\).*plugins/pay', line).group(1)
 
     # Kill pay.
     os.kill(int(pidstr), signal.SIGKILL)
-    wait_for(lambda: not n.daemon.running)
-
-    assert get_logfile_match(logpath, 'pay: Plugin marked as important, shutting down lightningd')
+    n.daemon.wait_for_log('pay: Plugin marked as important, shutting down lightningd')
+    # Should exit with exitcode 1
+    assert n.daemon.wait() == 1
+    n.stop()
 
 
 @pytest.mark.developer("tests developer-only option.")

tests/test_wallet.py

@@ -1038,12 +1038,11 @@ def test_hsm_secret_encryption(node_factory):
 
     # Test we cannot restore the same wallet with another password
     l1.daemon.opts.update({"encrypted-hsm": None})
-    l1.daemon.start(stdin=slave_fd, stderr=subprocess.STDOUT,
-                    wait_for_initialized=False)
+    l1.daemon.start(stdin=slave_fd, wait_for_initialized=False)
     l1.daemon.wait_for_log(r'Enter hsm_secret password')
     write_all(master_fd, password[2:].encode("utf-8"))
     assert(l1.daemon.proc.wait(WAIT_TIMEOUT) == HSM_BAD_PASSWORD)
-    assert(l1.daemon.is_in_log("Wrong password for encrypted hsm_secret."))
+    assert(l1.daemon.is_in_stderr("Wrong password for encrypted hsm_secret."))
 
     # Not sure why this helps, but seems to reduce flakiness where
     # tail() thread in testing/utils.py gets 'ValueError: readline of
@@ -1069,9 +1068,9 @@ def test_hsm_secret_encryption(node_factory):
 
 class HsmTool(TailableProc):
     """Helper for testing the hsmtool as a subprocess"""
-    def __init__(self, *args):
+    def __init__(self, directory, *args):
         self.prefix = "hsmtool"
-        TailableProc.__init__(self)
+        TailableProc.__init__(self, os.path.join(directory, "hsmtool"))
         assert hasattr(self, "env")
         self.cmd_line = ["tools/hsmtool", *args]
 
@@ -1098,9 +1097,8 @@ def test_hsmtool_secret_decryption(node_factory):
 
     # We can't use a wrong password !
     master_fd, slave_fd = os.openpty()
-    hsmtool = HsmTool("decrypt", hsm_path)
-    hsmtool.start(stdin=slave_fd,
-                  stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    hsmtool = HsmTool(node_factory.directory, "decrypt", hsm_path)
+    hsmtool.start(stdin=slave_fd)
     hsmtool.wait_for_log(r"Enter hsm_secret password:")
     write_all(master_fd, "A wrong pass\n\n".encode("utf-8"))
     hsmtool.proc.wait(WAIT_TIMEOUT)
@@ -1108,8 +1106,7 @@ def test_hsmtool_secret_decryption(node_factory):
 
     # Decrypt it with hsmtool
     master_fd, slave_fd = os.openpty()
-    hsmtool.start(stdin=slave_fd,
-                  stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    hsmtool.start(stdin=slave_fd)
     hsmtool.wait_for_log(r"Enter hsm_secret password:")
     write_all(master_fd, password.encode("utf-8"))
     assert hsmtool.proc.wait(WAIT_TIMEOUT) == 0
@@ -1122,9 +1119,8 @@ def test_hsmtool_secret_decryption(node_factory):
 
     # Test we can encrypt it offline
     master_fd, slave_fd = os.openpty()
-    hsmtool = HsmTool("encrypt", hsm_path)
-    hsmtool.start(stdin=slave_fd,
-                  stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    hsmtool = HsmTool(node_factory.directory, "encrypt", hsm_path)
+    hsmtool.start(stdin=slave_fd)
     hsmtool.wait_for_log(r"Enter hsm_secret password:")
     write_all(master_fd, password.encode("utf-8"))
     hsmtool.wait_for_log(r"Confirm hsm_secret password:")
@@ -1137,7 +1133,7 @@ def test_hsmtool_secret_decryption(node_factory):
 
     l1.daemon.opts.update({"encrypted-hsm": None})
     master_fd, slave_fd = os.openpty()
-    l1.daemon.start(stdin=slave_fd, stderr=subprocess.STDOUT,
+    l1.daemon.start(stdin=slave_fd,
                     wait_for_initialized=False)
 
     l1.daemon.wait_for_log(r'The hsm_secret is encrypted')
@@ -1149,9 +1145,8 @@ def test_hsmtool_secret_decryption(node_factory):
 
     # And finally test that we can also decrypt if encrypted with hsmtool
     master_fd, slave_fd = os.openpty()
-    hsmtool = HsmTool("decrypt", hsm_path)
-    hsmtool.start(stdin=slave_fd,
-                  stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    hsmtool = HsmTool(node_factory.directory, "decrypt", hsm_path)
+    hsmtool.start(stdin=slave_fd)
     hsmtool.wait_for_log(r"Enter hsm_secret password:")
     write_all(master_fd, password.encode("utf-8"))
     assert hsmtool.proc.wait(WAIT_TIMEOUT) == 0
@@ -1161,9 +1156,8 @@ def test_hsmtool_secret_decryption(node_factory):
 
     # We can roundtrip encryption and decryption using a password provided
     # through stdin.
-    hsmtool = HsmTool("encrypt", hsm_path)
-    hsmtool.start(stdin=subprocess.PIPE, stdout=subprocess.PIPE,
-                  stderr=subprocess.PIPE)
+    hsmtool = HsmTool(node_factory.directory, "encrypt", hsm_path)
+    hsmtool.start(stdin=subprocess.PIPE)
     hsmtool.proc.stdin.write(password.encode("utf-8"))
     hsmtool.proc.stdin.write(password.encode("utf-8"))
     hsmtool.proc.stdin.flush()
@@ -1171,9 +1165,8 @@ def test_hsmtool_secret_decryption(node_factory):
     assert hsmtool.proc.wait(WAIT_TIMEOUT) == 0
 
     master_fd, slave_fd = os.openpty()
-    hsmtool = HsmTool("decrypt", hsm_path)
-    hsmtool.start(stdin=slave_fd,
-                  stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    hsmtool = HsmTool(node_factory.directory, "decrypt", hsm_path)
+    hsmtool.start(stdin=slave_fd)
     hsmtool.wait_for_log("Enter hsm_secret password:")
     write_all(master_fd, password.encode("utf-8"))
     hsmtool.wait_for_log("Successfully decrypted")
@@ -1232,19 +1225,17 @@ def test_hsmtool_generatehsm(node_factory):
     hsm_path = os.path.join(l1.daemon.lightning_dir, TEST_NETWORK,
                             "hsm_secret")
 
-    hsmtool = HsmTool("generatehsm", hsm_path)
+    hsmtool = HsmTool(node_factory.directory, "generatehsm", hsm_path)
 
     # You cannot re-generate an already existing hsm_secret
     master_fd, slave_fd = os.openpty()
-    hsmtool.start(stdin=slave_fd, stdout=subprocess.PIPE,
-                  stderr=subprocess.PIPE)
+    hsmtool.start(stdin=slave_fd)
     assert hsmtool.proc.wait(WAIT_TIMEOUT) == 2
     os.remove(hsm_path)
 
     # We can generate a valid hsm_secret from a wordlist and a "passphrase"
     master_fd, slave_fd = os.openpty()
-    hsmtool.start(stdin=slave_fd, stdout=subprocess.PIPE,
-                  stderr=subprocess.PIPE)
+    hsmtool.start(stdin=slave_fd)
     hsmtool.wait_for_log(r"Select your language:")
     write_all(master_fd, "0\n".encode("utf-8"))
     hsmtool.wait_for_log(r"Introduce your BIP39 word list")