Chore: Node manager refactoring (#1974)

* refactoring of node manager + tests
Co-authored-by: k9ert <[email protected]>

src/cryptoadvance/specter/managers/node_manager.py

@@ -30,6 +30,7 @@ def __init__(
         data_folder="",
     ):
         self.nodes = {}
+        # Dict is sth. like: {'nigiri_regtest': <Node name=Nigiri regtest fullpath=...>, 'default': <Node name=Bitcoin Core fullpath=...>}
         self.data_folder = data_folder
         self._active_node = active_node
         self.proxy_url = proxy_url
@@ -51,22 +52,23 @@ def load_from_disk(self, data_folder=None):
             # creating folders if they don't exist
             if not os.path.isdir(data_folder):
                 os.mkdir(data_folder)
-        nodes_files = load_jsons(self.data_folder, key="name")
+        nodes_files = load_jsons(self.data_folder, key="alias")
         for node_alias in nodes_files:
             try:
-                self.nodes[
-                    nodes_files[node_alias]["name"]
-                ] = PersistentObject.from_json(
+                self.nodes[node_alias] = PersistentObject.from_json(
                     nodes_files[node_alias],
                     self,
-                    default_alias=node_alias,
+                    default_alias=nodes_files[node_alias]["alias"],
                     default_fullpath=calc_fullpath(self.data_folder, node_alias),
                 )
             except SpecterInternalException as e:
                 logger.error(f"Skipping node {node_alias} due to {e}")
 
         if not self.nodes:
             if os.environ.get("ELM_RPC_USER"):
+                logger.debug(
+                    "Creating an external Elements node with the initial configuration."
+                )
                 self.add_external_node(
                     node_type="ELM",
                     name="Blockstream Liquid",
@@ -79,7 +81,9 @@ def load_from_disk(self, data_folder=None):
                     protocol="http",
                     default_alias=self.DEFAULT_ALIAS,
                 )
-            logger.info("Creating initial node-configuration")
+            logger.debug(
+                "Creating an external BTC node with the initial configuration."
+            )
             self.add_external_node(
                 node_type="BTC",
                 name="Bitcoin Core",
@@ -93,46 +97,56 @@ def load_from_disk(self, data_folder=None):
                 default_alias=self.DEFAULT_ALIAS,
             )
 
-        # Just to be sure here ....
+        # Make sure we always have the default node
+        # (needed for the rpc-as-pin-authentication used on Raspiblitz)
         has_default_node = False
-        for name, node in self.nodes.items():
+        for node in self.nodes.values():
             if node.alias == self.DEFAULT_ALIAS:
-                return
-        # Make sure we always have a default node
-        # (needed for the rpc-as-pin-authentication, created and used for raspiblitz)
-        self.add_external_node(
-            node_type="BTC",
-            name="Bitcoin Core",
-            autodetect=True,
-            datadir=get_default_datadir(),
-            user="",
-            password="",
-            port=8332,
-            host="localhost",
-            protocol="http",
-            default_alias=self.DEFAULT_ALIAS,
-        )
+                has_default_node = True
+        # Recreate the default node if it doesn't exist anymore
+        if not has_default_node:
+            logger.debug("Recreating the default node.")
+            self.add_external_node(
+                node_type="BTC",
+                name="Bitcoin Core",
+                autodetect=True,
+                datadir=get_default_datadir(),
+                user="",
+                password="",
+                port=8332,
+                host="localhost",
+                protocol="http",
+                default_alias=self.DEFAULT_ALIAS,
+            )
 
     @property
-    def active_node(self):
+    def active_node(self) -> Node:
         return self.get_by_alias(self._active_node)
 
     @property
-    def nodes_names(self):
-        return sorted(self.nodes.keys())
+    def nodes_names(self) -> list:
+        """Returns a list of the names (not the aliases) of the nodes"""
+        return [node.name for node in self.nodes.values()]
 
-    def switch_node(self, node_alias):
-        # this will throw an error if the node doesn't exist
+    def switch_node(self, node_alias: str):
+        # This will throw an error if the node doesn't exist
+        logger.debug(f"Switching from {self._active_node} to {node_alias}.")
         self._active_node = self.get_by_alias(node_alias).alias
 
-    def default_node(self):
+    def default_node(self) -> Node:
         return self.get_by_alias(self.DEFAULT_ALIAS)
 
-    def get_by_alias(self, alias):
-        for node_name in self.nodes:
-            if self.nodes[node_name] and self.nodes[node_name].alias == alias:
-                return self.nodes[node_name]
-        raise SpecterError("Node %s does not exist!" % alias)
+    def get_by_alias(self, alias: str) -> Node:
+        for node in self.nodes.values():
+            if node.alias == alias:
+                return node
+        raise SpecterError("Node alias %s does not exist!" % alias)
+
+    def get_by_name(self, name: str) -> Node:
+        for node in self.nodes.values():
+            if node.name == name:
+                return node
+        raise SpecterError("Node name %s does not exist!" % name)
 
     def update_bitcoind_version(self, specter, version):
         stopped_nodes = []
@@ -168,7 +182,7 @@ def add_external_node(
         """Adding a node. Params:
         :param node_type: only valid for autodetect. Either BTC or ELM
         This should only be used for an external node. Use add_internal_node for internal node
-        and if you have defined your own node-type, use save_node directly. to save the node (and create it yourself)
+        and if you have defined your own node type, use save_node directly to save the node (and create it yourself)
         """
         if not default_alias:
             node_alias = alias(name)
@@ -195,20 +209,19 @@ def add_external_node(
             node_type,
             self,
         )
-        logger.info(f"persisting {node} in add_external_node")
-        self.nodes[name] = node
-        return self.save_node(node)
+        logger.debug(f"Persisting {node_alias} from an add_external_node call.")
+        self.nodes[node_alias] = node
+        self.save_node(node)
+        return node
 
     def save_node(self, node):
         fullpath = (
             node.fullpath
             if hasattr(node, "fullpath")
-            else calc_fullpath(self.data_folder, node.name)
+            else calc_fullpath(self.data_folder, node.alias)
         )
         write_node(node, fullpath)
-
         logger.info("Added new node {}".format(node.alias))
-        return node
 
     def add_internal_node(
         self,
@@ -251,15 +264,22 @@ def add_internal_node(
             network,
             self.internal_bitcoind_version,
         )
-        self.nodes[name] = node
-        return self.save_node(node)
+        self.nodes[node_alias] = node
+        return node
 
     def delete_node(self, node, specter):
         logger.info("Deleting {}".format(node.alias))
-        # Delete files
-        delete_file(node.fullpath)
-        delete_file(node.fullpath + ".bkp")
-        if self._active_node == node.alias:
-            specter.update_active_node(next(iter(self.nodes.values())).alias)
-        del self.nodes[node.name]
-        logger.info("Node {} was deleted successfully".format(node.alias))
+        try:
+            # Delete from wallet manager
+            del self.nodes[node.alias]
+            # Delete files
+            delete_file(node.fullpath)
+            delete_file(node.fullpath + ".bkp")
+            # Update the active node
+            if self._active_node == node.alias:
+                specter.update_active_node(
+                    next(iter(self.nodes.values())).alias
+                )  # This switches to the first node in the node list, which is usually the default node
+            logger.info("Node {} was deleted successfully".format(node.alias))
+        except KeyError:
+            raise SpecterError(f"{node.name} not found, node could not be deleted.")

src/cryptoadvance/specter/server_endpoints/nodes.py

@@ -166,7 +166,7 @@ def node_settings(node_alias):
                 flash(_("Test failed: {}").format(test["err"]), "error")
         elif action == "save":
             if not node_alias:
-                if node.name in app.specter.node_manager.nodes:
+                if node.name in app.specter.node_manager.nodes_names:
                     flash(
                         _(
                             "Node with this name already exists, please choose a different name."
@@ -271,7 +271,7 @@ def internal_node_settings(node_alias):
                 node.rename(node_name)
         elif action == "forget":
             if not node_alias:
-                flash(_("Failed to deleted node. Node isn't saved"), "error")
+                flash(_("Failed to delete node. Node isn't saved"), "error")
             elif len(app.specter.node_manager.nodes) > 1:
                 node.stop()
                 app.specter.node_manager.delete_node(node, app.specter)

src/cryptoadvance/specter/server_endpoints/setup.py

@@ -161,7 +161,7 @@ def setup_bitcoind_datadir():
     network = request.form.get("network", "main")
     node_name = "Specter Bitcoin" if network == "main" else f"Specter {network.title()}"
     i = 1
-    while node_name in app.specter.node_manager.nodes:
+    while node_name in app.specter.node_manager.nodes_names:
         i += 1
         node_name = (
             f"Specter Bitcoin {i}"

src/cryptoadvance/specter/templates/includes/sidebar/components/node_select_popup.jinja

@@ -25,7 +25,7 @@
 <div id="node_select_popup" class="hidden" style="text-align: left;">
     <h1>{{ _("Node configuration") }}</h1>
     {% for node_name in specter.node_manager.nodes_names %}
-        {% set node = specter.node_manager.nodes[node_name] %}
+        {% set node = specter.node_manager.get_by_name(node_name) %}
         <form action="{{url_for('nodes_endpoint.switch_node')}}" id="{{node.alias}}-select-node-form" method="POST">
 		    <input type="hidden" class="csrf-token" name="csrf_token" value="{{ csrf_token() }}"/>
 		    <input type="hidden" name="node_alias" value="{{ node.alias }}"/>

src/cryptoadvance/specter/util/migrations/migration_0002.py

@@ -41,7 +41,11 @@ def description(self) -> str:
             In order to reverse this migration, you do need to reverese the addition of the 
             python_class like this:
             
+            sudo apt-get install moreutils jq
             cd ~/.specter/nodes
+            # adding the external_node key again:
+            for file in `ls *.json`; do jq '. | select(.python_class=="cryptoadvance.specter.internal_node.InternalNode") + {"external_node":false} , . | select(.python_class=="cryptoadvance.specter.node.Node") + {"external_node":true}'  $file | sponge $file ; done
+            # remove python_class key
             for file in `ls *.json`; do jq 'del(.python_class)' $file | sponge $file ; done
             cd ..
             jq 'del(.migration_executions[] | select(.migration_id == 2))' migration_data.json | sponge migration_data.json
@@ -55,7 +59,11 @@ def execute(self):
             return
 
         nodes = {}
-        nodes_files = load_jsons(node_folder, key="name")
+        logger.info(f"Loading all json-files from {node_folder}")
+        nodes_files = load_jsons(node_folder, key="alias")
+        logger.info(
+            f"iterating these nodes: {[node_alias for node_alias in nodes_files ]}"
+        )
         for node_alias in nodes_files:
 
             fullpath = os.path.join(self.data_folder, "%s.json" % node_alias)

tests/conftest.py

@@ -218,18 +218,39 @@ def node(empty_data_folder, bitcoin_regtest):
     nodes_folder = empty_data_folder + "/nodes"
     if not os.path.isdir(nodes_folder):
         os.makedirs(nodes_folder)
-    node = Node.from_json(
-        {
-            "autodetect": False,
-            "datadir": bitcoin_regtest.datadir,
-            "user": bitcoin_regtest.rpcconn.rpcuser,
-            "password": bitcoin_regtest.rpcconn.rpcpassword,
-            "port": bitcoin_regtest.rpcconn.rpcport,
-            "host": bitcoin_regtest.rpcconn.ipaddress,
-            "protocol": "http",
-        },
-        manager=NodeManager(data_folder=nodes_folder),
-        default_fullpath=os.path.join(nodes_folder, "standard_node.json"),
+    nm = NodeManager(data_folder=nodes_folder)
+    node = nm.add_external_node(
+        "BTC",
+        "Standard node",
+        False,
+        bitcoin_regtest.datadir,
+        bitcoin_regtest.rpcconn.rpcuser,
+        bitcoin_regtest.rpcconn.rpcpassword,
+        bitcoin_regtest.rpcconn.rpcport,
+        bitcoin_regtest.rpcconn._ipaddress,
+        "http",
+        "standard_node",
+    )
+    return node
+
+
+@pytest.fixture
+def node_with_different_port(empty_data_folder, bitcoin_regtest):
+    nodes_folder = empty_data_folder + "/nodes"
+    if not os.path.isdir(nodes_folder):
+        os.makedirs(nodes_folder)
+    nm = NodeManager(data_folder=nodes_folder)
+    node = nm.add_external_node(
+        "BTC",
+        "Node with a different port",
+        False,
+        "",
+        bitcoin_regtest.rpcconn.rpcuser,
+        bitcoin_regtest.rpcconn.rpcpassword,
+        18333,
+        bitcoin_regtest.rpcconn._ipaddress,
+        "http",
+        "satoshis_node",
     )
     return node