Merge #38: lnprototest: refactroing abstract class and fixed the #14

66b5a94 lnprototest: remove the default __enter__ __exit__ implementation (Vincenzo Palazzo)
fd9bae5 lnprototest: remove info logging when it is not necessary (Vincenzo Palazzo)
c7144c5 doc: adding log information in the  Readme (Vincenzo Palazzo)
8ff83a9 ci: during the test in the ci increase verbosity of the tests (Vincenzo Palazzo)
363b9e5 lnprototest: adding teardown directory at the end of the tests (Vincenzo Palazzo)
4b4823c fmt: formatting code (Vincenzo Palazzo)
e0cce0a lnprototest: refactroing abstract class and fixed the #14 (Vincenzo Palazzo)

Pull request description:

  kill all the processes when the class was removed from the scope.

  Fixes #14

  This is a refactoring PR cherry-pitched by the PR #37 but it is cleaner to introduce these changes in another PR.

  Signed-off-by: Vincenzo Palazzo <[email protected]>

Top commit has no ACKs.

Tree-SHA512: f2bd5f609caecd9a9bb3bfcd4f02fe7bd73a69a63265c5ee9fca73b7c34116ea044c84fabec0c3d009f7604e63121c0d18fa0f54d5eda19cf730130d8f55d0d1

README.md

@@ -54,6 +54,7 @@ Here are some other useful pytest options:
 5. `-k foo` to only run tests with 'foo' in their name.
 6. `tests/test_bolt1-01-init.py` to only run tests in that file.
 7. `tests/test_bolt1-01-init.py::test_init` to only run that test.
+8. `--log-cli-level={LEVEL_NAME}` to enable the logging during the test execution.
 
 ### Running Against A Real Node.
 

docker/entrypoint.sh

@@ -1,3 +1,3 @@
 #! /bin/bash
 cd lnprototest
-make check PYTEST_ARGS='--runner=lnprototest.clightning.Runner'
+make check PYTEST_ARGS='--runner=lnprototest.clightning.Runner --log-cli-level=DEBUG'

lnprototest/backend/bitcoind.py

@@ -57,9 +57,11 @@ class Bitcoind(Backend):
     """Starts regtest bitcoind on an ephemeral port, and returns the RPC proxy"""
 
     def __init__(self, basedir: str):
+        self.rpc = None
+        self.proc = None
+        self.base_dir = basedir
+        logging.debug(f"Base dir is {basedir}")
         self.bitcoin_dir = os.path.join(basedir, "bitcoind")
-        if not os.path.exists(self.bitcoin_dir):
-            os.makedirs(self.bitcoin_dir)
         self.bitcoin_conf = os.path.join(self.bitcoin_dir, "bitcoin.conf")
         self.cmd_line = [
             "bitcoind",
@@ -71,9 +73,17 @@ def __init__(self, basedir: str):
         ]
         self.port = reserve()
         self.btc_version = None
-        print("Port is {}, dir is {}".format(self.port, self.bitcoin_dir))
+        logging.debug("Port is {}, dir is {}".format(self.port, self.bitcoin_dir))
+
+    def __init_bitcoin_conf(self):
+        """Init the bitcoin core directory with all the necessary information
+        to startup the node"""
+        if not os.path.exists(self.bitcoin_dir):
+            os.makedirs(self.bitcoin_dir)
+            logging.debug(f"Creating {self.bitcoin_dir} directory")
         # For after 0.16.1 (eg. 3f398d7a17f136cd4a67998406ca41a124ae2966), this
         # needs its own [regtest] section.
+        logging.debug(f"Writing bitcoin conf file at {self.bitcoin_conf}")
         with open(self.bitcoin_conf, "w") as f:
             f.write("regtest=1\n")
             f.write("rpcuser=rpcuser\n")
@@ -82,35 +92,51 @@ def __init__(self, basedir: str):
             f.write("rpcport={}\n".format(self.port))
         self.rpc = BitcoinProxy(btc_conf_file=self.bitcoin_conf)
 
-    def version_compatibility(self) -> None:
+    def __version_compatibility(self) -> None:
         """
-        This method try to manage the compatibility between
-        different version of Bitcoin Core implementation.
+        This method tries to manage the compatibility between
+        different versions of Bitcoin Core implementation.
 
-        This method could be useful sometimes when is necessary
-        run the test with different version of Bitcoin core.
+        This method could sometimes be useful when it is necessary to
+        run the test with a different version of Bitcoin core.
         """
         if self.rpc is None:
             # Sanity check
             raise ValueError("bitcoind not initialized")
 
         self.btc_version = self.rpc.getnetworkinfo()["version"]
         assert self.btc_version is not None
-        logging.info("Bitcoin Core version {}".format(self.btc_version))
+        logging.debug("Bitcoin Core version {}".format(self.btc_version))
         if self.btc_version >= 210000:
             # Maintains the compatibility between wallet
             # different ln implementation can use the main wallet (?)
             self.rpc.createwallet("main")  # Automatically loads
 
+    def __is__bitcoind_ready(self) -> bool:
+        """Check if bitcoind is ready during the execution"""
+        if self.rpc is None:
+            # Sanity check
+            raise ValueError("bitcoind not initialized")
+
+        try:
+            self.btc_version = self.rpc.getnetworkinfo()
+            return True
+        except Exception as ex:
+            logging.debug(f"{ex}")
+            return False
+
     def start(self) -> None:
+        if self.rpc is None:
+            self.__init_bitcoin_conf()
+        # TODO: We can move this to a single call and not use Popen
         self.proc = subprocess.Popen(self.cmd_line, stdout=subprocess.PIPE)
         assert self.proc.stdout
 
         # Wait for it to startup.
-        while b"Done loading" not in self.proc.stdout.readline():
-            pass
+        while not self.__is__bitcoind_ready():
+            logging.debug("Bitcoin core is loading")
 
-        self.version_compatibility()
+        self.__version_compatibility()
         # Block #1.
         # Privkey the coinbase spends to:
         #    cUB4V7VCk6mX32981TWviQVLkj3pa2zBcXrjMZ9QwaZB5Kojhp59
@@ -121,10 +147,10 @@ def start(self) -> None:
 
     def stop(self) -> None:
         self.proc.kill()
+        shutil.rmtree(os.path.join(self.bitcoin_dir, "regtest"))
 
     def restart(self) -> None:
         # Only restart if we have to.
         if self.rpc.getblockcount() != 101 or self.rpc.getrawmempool() != []:
             self.stop()
-            shutil.rmtree(os.path.join(self.bitcoin_dir, "regtest"))
             self.start()

lnprototest/clightning/clightning.py

@@ -9,10 +9,11 @@
 import pyln.proto.wire
 import os
 import subprocess
-import tempfile
 import lnprototest
 import bitcoin.core
 import struct
+import shutil
+import logging
 
 from concurrent import futures
 from ephemeral_port_reserve import reserve
@@ -53,18 +54,14 @@ def __init__(self, connprivkey: str, port: int):
 class Runner(lnprototest.Runner):
     def __init__(self, config: Any):
         super().__init__(config)
+        self.running = False
+        self.rpc = None
+        self.bitcoind = None
+        self.proc = None
         self.cleanup_callbacks: List[Callable[[], None]] = []
         self.fundchannel_future: Optional[Any] = None
         self.is_fundchannel_kill = False
-
-        directory = tempfile.mkdtemp(prefix="lnpt-cl-")
-        self.bitcoind = Bitcoind(directory)
-        self.bitcoind.start()
         self.executor = futures.ThreadPoolExecutor(max_workers=20)
-
-        self.lightning_dir = os.path.join(directory, "lightningd")
-        if not os.path.exists(self.lightning_dir):
-            os.makedirs(self.lightning_dir)
         self.lightning_port = reserve()
 
         self.startup_flags = []
@@ -91,6 +88,12 @@ def __init__(self, config: Any):
                 k, v = o.split("/")
                 self.options[k] = v
 
+    def __init_sandbox_dir(self):
+        """Create the tmp directory for lnprotest and lightningd"""
+        self.lightning_dir = os.path.join(self.directory, "lightningd")
+        if not os.path.exists(self.lightning_dir):
+            os.makedirs(self.lightning_dir)
+
     def get_keyset(self) -> KeySet:
         return KeySet(
             revocation_base_secret="0000000000000000000000000000000000000000000000000000000000000011",
@@ -106,7 +109,13 @@ def get_node_privkey(self) -> str:
     def get_node_bitcoinkey(self) -> str:
         return "0000000000000000000000000000000000000000000000000000000000000010"
 
+    def is_running(self) -> bool:
+        return self.running
+
     def start(self) -> None:
+        self.logger.debug("[START]")
+        self.__init_sandbox_dir()
+        self.bitcoind = Bitcoind(self.directory)
         self.proc = subprocess.Popen(
             [
                 "{}/lightningd/lightningd".format(LIGHTNING_SRC),
@@ -128,72 +137,66 @@ def start(self) -> None:
             ]
             + self.startup_flags
         )
+        self.running = True
+        try:
+            self.bitcoind.start()
+        except Exception as ex:
+            self.logger.debug(f"Exception with message {ex}")
+        self.logger.debug("RUN Bitcoind")
         self.rpc = pyln.client.LightningRpc(
             os.path.join(self.lightning_dir, "regtest", "lightning-rpc")
         )
+        self.logger.debug("RUN c-lightning")
 
         def node_ready(rpc: pyln.client.LightningRpc) -> bool:
             try:
                 rpc.getinfo()
                 return True
-            except Exception:
+            except Exception as ex:
+                logging.debug(f"waiting for c-lightning: Exception received {ex}")
                 return False
 
         wait_for(lambda: node_ready(self.rpc))
+        logging.debug("Waited fro c-lightning")
 
         # Make sure that we see any funds that come to our wallet
         for i in range(5):
             self.rpc.newaddr()
 
-    def kill_fundchannel(self) -> None:
-        fut = self.fundchannel_future
-        self.fundchannel_future = None
-        self.is_fundchannel_kill = True
-        if fut:
-            try:
-                fut.result(0)
-            except (SpecFileError, futures.TimeoutError):
-                pass
-
     def shutdown(self) -> None:
         for cb in self.cleanup_callbacks:
             cb()
-
-    def stop(self) -> None:
-        for cb in self.cleanup_callbacks:
-            cb()
         self.rpc.stop()
         self.bitcoind.stop()
-        for c in self.conns.values():
-            cast(CLightningConn, c).connection.connection.close()
-
-    def connect(self, event: Event, connprivkey: str) -> None:
-        self.add_conn(CLightningConn(connprivkey, self.lightning_port))
 
-    def __enter__(self) -> "Runner":
-        self.start()
-        return self
-
-    def __exit__(self, type: Any, value: Any, tb: Any) -> None:
-        self.stop()
-
-    def restart(self) -> None:
-        if self.config.getoption("verbose"):
-            print("[RESTART]")
-        for cb in self.cleanup_callbacks:
-            cb()
-        self.rpc.stop()
-        self.bitcoind.restart()
+    def stop(self) -> None:
+        self.logger.debug("[STOP]")
+        self.shutdown()
+        self.running = False
         for c in self.conns.values():
             cast(CLightningConn, c).connection.connection.close()
+        shutil.rmtree(os.path.join(self.lightning_dir, "regtest"))
 
+    def restart(self) -> None:
+        self.logger.debug("[RESTART]")
+        self.stop()
         # Make a clean start
-        os.remove(os.path.join(self.lightning_dir, "regtest", "gossip_store"))
-        os.remove(os.path.join(self.lightning_dir, "regtest", "lightningd.sqlite3"))
-        os.remove(os.path.join(self.lightning_dir, "regtest", "log"))
         super().restart()
         self.start()
 
+    def kill_fundchannel(self) -> None:
+        fut = self.fundchannel_future
+        self.fundchannel_future = None
+        self.is_fundchannel_kill = True
+        if fut:
+            try:
+                fut.result(0)
+            except (SpecFileError, futures.TimeoutError):
+                pass
+
+    def connect(self, event: Event, connprivkey: str) -> None:
+        self.add_conn(CLightningConn(connprivkey, self.lightning_port))
+
     def getblockheight(self) -> int:
         return self.bitcoind.rpc.getblockcount()
 
@@ -435,3 +438,13 @@ def add_startup_flag(self, flag: str) -> None:
         if self.config.getoption("verbose"):
             print("[ADD STARTUP FLAG '{}']".format(flag))
         self.startup_flags.append("--{}".format(flag))
+
+    def close_channel(self, channel_id: str) -> bool:
+        if self.config.getoption("verbose"):
+            print("[CLOSE CHANNEL '{}']".format(channel_id))
+        try:
+            self.rpc.close(peer_id=channel_id)
+        except Exception as ex:
+            print(ex)
+            return False
+        return True

lnprototest/dummyrunner.py

@@ -207,3 +207,14 @@ def check_final_error(
         must_not_events: List[MustNotMsg],
     ) -> None:
         pass
+
+    def close_channel(self, channel_id: str) -> bool:
+        if self.config.getoption("verbose"):
+            print("[CLOSE-CHANNEL {}]".format(channel_id))
+        return True
+
+    def is_running(self) -> bool:
+        return True
+
+    def teardown(self):
+        pass