Bugfix: Incomplete wallet deletion (#1950)

Also:
* threading for testing via specter config
* helpful output for flaky tests

Co-authored-by: k9ert <[email protected]>

Author: moneymanolis

pytest.ini

@@ -6,6 +6,8 @@ markers =
     slow: mark test as slow.
     elm: mark test as elementsd dependent
     bottleneck: mark a test as so ressource intensive that it can create a bottleneck where the test just fails due to a lack of ressources
+    threading: test needs threading to work
+    
 # If you need live logging to debug, uncomment the next line
 # log_cli = True 
 # Then set the desired logging level on the command line, for example:

src/cryptoadvance/specter/managers/config_manager.py

@@ -67,6 +67,9 @@ def __init__(self, data_folder, config={}):
             "hide_sensitive_info": False,
             "autohide_sensitive_info_timeout_minutes": None,
             "autologout_timeout_hours": 4,
+            "testing": {
+                "allow_threading_for_testing": False,
+            },
             # TODO: remove
             "bitcoind": False,
         }

src/cryptoadvance/specter/managers/wallet_manager.py

@@ -1,14 +1,13 @@
 import logging
 import os
+import sys
 import pathlib
-import shutil
 import threading
 import traceback
 from typing import Dict
 
 from flask_babel import lazy_gettext as _
 from cryptoadvance.specter.rpc import BitcoinRPC
-
 from cryptoadvance.specter.key import Key
 
 from ..helpers import add_dicts, alias, is_liquid, load_jsons
@@ -37,7 +36,7 @@ def __init__(
         chain,
         device_manager,
         path="specter",
-        allow_threading=True,
+        allow_threading_for_testing=False,
     ):
         self.data_folder = data_folder
         self.chain = chain
@@ -52,7 +51,7 @@ def __init__(
         self.wallets = {}
         # A way to communicate failed wallets to the outside
         self.bitcoin_core_version_raw = bitcoin_core_version_raw
-        self.allow_threading = allow_threading
+        self.allow_threading_for_testing = allow_threading_for_testing
         # define different wallet classes for liquid and bitcoin
         self.WalletClass = LWallet if is_liquid(chain) else Wallet
         self.update(data_folder, rpc, chain)
@@ -108,15 +107,29 @@ def update(
             and self.rpc is not None
             and self.chain is not None
         ):
-            if self.allow_threading and use_threading:
-                t = threading.Thread(
-                    target=self._update,
-                    args=(wallets_update_list,),
-                )
-
-                t.start()
+            if "pytest" in sys.modules:
+                if self.allow_threading_for_testing:
+                    logger.info("Using threads in updating the wallet manager.")
+                    t = threading.Thread(
+                        target=self._update,
+                        args=(wallets_update_list,),
+                    )
+                    t.start()
+                else:
+                    logger.info("Not using threads in updating the wallet manager.")
+                    self._update(wallets_update_list)
             else:
-                self._update(wallets_update_list)
+                if use_threading:
+                    logger.info("Using threads in updating the wallet manager.")
+                    t = threading.Thread(
+                        target=self._update,
+                        args=(wallets_update_list,),
+                    )
+
+                    t.start()
+                else:
+                    logger.info("Not using threads in updating the wallet manager.")
+                    self._update(wallets_update_list)
         else:
             self.is_loading = False
             logger.warning(
@@ -134,7 +147,7 @@ def _update(self, wallets_update_list: Dict):
         * and, on the Specter side, the wallet objects of those unloaded wallets are reinitialised
         """
         logger.info(
-            f"Started Updating Wallets with {len(wallets_update_list.values())} wallets"
+            f"Started updating wallets with {len(wallets_update_list.values())} wallets"
         )
         # list of wallets in the dict
         existing_names = list(self.wallets.keys())
@@ -147,7 +160,7 @@ def _update(self, wallets_update_list: Dict):
 
                     wallet_alias = wallets_update_list[wallet]["alias"]
                     wallet_name = wallets_update_list[wallet]["name"]
-                    logger.info(f"  Updating wallet {wallet_name}")
+                    logger.info(f"Updating wallet {wallet_name}")
                     # wallet from json not yet loaded in Bitcoin Core?!
                     if os.path.join(self.rpc_path, wallet_alias) not in loaded_wallets:
                         try:
@@ -348,27 +361,43 @@ def create_wallet(self, name, sigs_required, key_type, keys, devices, **kwargs):
         else:
             raise ("Failed to create new wallet")
 
-    def delete_wallet(
-        self, wallet, bitcoin_datadir=get_default_datadir(), chain="main"
-    ):
-        logger.info("Deleting {}".format(wallet.alias))
-        wallet_rpc_path = os.path.join(self.rpc_path, wallet.alias)
-        self.rpc.unloadwallet(wallet_rpc_path)
-        # Try deleting wallet folder
-        if bitcoin_datadir:
-            if chain != "main":
-                bitcoin_datadir = os.path.join(bitcoin_datadir, chain)
-            candidates = [
-                os.path.join(bitcoin_datadir, wallet_rpc_path),
-                os.path.join(bitcoin_datadir, "wallets", wallet_rpc_path),
-            ]
-            for path in candidates:
-                shutil.rmtree(path, ignore_errors=True)
-
-        # Delete files
-        wallet.delete_files()
-        del self.wallets[wallet.name]
-        self.update()
+    def delete_wallet(self, wallet, node=None) -> tuple:
+        """Returns a tuple with two Booleans, indicating whether the wallet was deleted on the Specter side and/or on the node side."""
+        logger.info(f"Deleting {wallet.alias}")
+        specter_wallet_deleted = False
+        node_wallet_file_deleted = False
+        # Make first sure that we can unload the wallet in Bitcoin Core
+        try:
+            wallet_rpc_path = os.path.join(
+                self.rpc_path, wallet.alias
+            )  # e.g. specter/jade_wallet
+            logger.debug(f"The wallet_rpc_path is: {wallet_rpc_path}")
+            self.rpc.unloadwallet(wallet_rpc_path)
+            # Delete the wallet.json and backups
+            try:
+                wallet.delete_files()
+                # Remove the wallet instance
+                del self.wallets[wallet.name]
+                self.update()
+                specter_wallet_deleted = True
+            except KeyError:
+                raise SpecterError(
+                    f"The wallet {wallet.name} has already been deleted."
+                )
+            except SpecterInternalException as sie:
+                logger.exception(
+                    f"Could not delete the wallet {wallet.name} in Specter due to {sie}"
+                )
+            # Also delete the wallet file on the node if possible
+            if node:
+                if node.delete_wallet_file(wallet):
+                    node_wallet_file_deleted = True
+        except RpcError:
+            raise SpecterError(
+                "Unable to unload the wallet on the node. Aborting the deletion of the wallet ..."
+            )
+        deleted = (specter_wallet_deleted, node_wallet_file_deleted)
+        return deleted
 
     def rename_wallet(self, wallet, name):
         logger.info("Renaming {}".format(wallet.alias))
@@ -469,7 +498,7 @@ def delete(self, specter):
         """Deletes all the wallets"""
         for w in list(self.wallets.keys()):
             wallet = self.wallets[w]
-            self.delete_wallet(wallet, specter.bitcoin_datadir, specter.chain)
+            self.delete_wallet(wallet)
         delete_folder(self.data_folder)
 
     @classmethod

src/cryptoadvance/specter/node.py

@@ -2,6 +2,7 @@
 import logging
 import os
 from os import path
+import shutil
 
 from embit.liquid.networks import get_network
 from flask import render_template
@@ -17,7 +18,7 @@
     autodetect_rpc_confs,
     get_default_datadir,
 )
-from .specter_error import BrokenCoreConnectionException
+from .specter_error import SpecterError, BrokenCoreConnectionException
 
 logger = logging.getLogger(__name__)
 
@@ -534,6 +535,44 @@ def check_blockheight(self):
     def is_liquid(self):
         return is_liquid(self.chain)
 
+    def delete_wallet_file(self, wallet) -> bool:
+        """Deleting the wallet file located on the node. This only works if the node is on the same machine as Specter.
+        Returns True if the wallet file could be deleted, otherwise returns False."""
+        datadir = ""
+        if self.datadir == "":
+            # In case someone did not toggle the auto-detect but still used the default location.
+            # When you set up a new node and deactivate the auto-detect, the datadir is set to an empty string.
+            logger.debug(
+                f"The node datadir before get_default_datadir is: {self.datadir}"
+            )
+            datadir = get_default_datadir(self.node_type)
+            logger.debug(f"The node datadir after get_default_datadir is: {datadir}")
+        else:
+            datadir = self.datadir
+        wallet_file_removed = False
+        path = ""
+        # Check whether wallet was really unloaded
+        wallet_rpc_path = os.path.join(wallet.manager.rpc_path, wallet.alias)
+        # If we can unload the wallet via RPC it had not been unloaded properly before by the wallet manager
+        try:
+            self.rpc.unloadwallet(wallet_rpc_path)
+            raise SpecterError(
+                "Trying to delete the wallet file on the node but the wallet had not been unloaded properly."
+            )
+        except RpcError:
+            pass
+        if self.chain != "main":
+            path = os.path.join(datadir, f"{self.chain}/wallets", wallet_rpc_path)
+        else:
+            path = os.path.join(datadir, wallet_rpc_path)
+        try:
+            shutil.rmtree(path, ignore_errors=False)
+            logger.debug(f"Removing wallet file at: {path}")
+            wallet_file_removed = True
+        except FileNotFoundError:
+            logger.debug(f"Could not find any wallet file at: {path}")
+        return wallet_file_removed
+
     @property
     def is_running(self):
         if self._network_info["version"] == 999999:
@@ -619,6 +658,10 @@ def node_type(self):
             return self._node_type
         return "BTC"
 
+    @property
+    def default_datadir(self):
+        return get_default_datadir(self.node_type)
+
     @rpc.setter
     def rpc(self, value):
         if hasattr(self, "_rpc") and self._rpc != value:

src/cryptoadvance/specter/server_endpoints/wallets/wallets.py

@@ -850,11 +850,28 @@ def settings(wallet_alias):
             )
             wallet.getdata()
         elif action == "deletewallet":
-            app.specter.wallet_manager.delete_wallet(
-                wallet, app.specter.bitcoin_datadir, app.specter.chain
-            )
-            response = redirect(url_for("index"))
-            return response
+            deleted = app.specter.wallet_manager.delete_wallet(wallet, app.specter.node)
+            # deleted is a tuple: (specter_wallet_deleted, core_wallet_file_deleted)
+            if deleted == (True, True):
+                flash(
+                    _("Wallet in Specter and wallet file on node deleted successfully.")
+                )
+            elif deleted == (True, False):
+                flash(
+                    _(
+                        "Wallet in Specter deleted successfully but wallet file on node could not be removed automatically."
+                    )
+                )
+            elif deleted == (False, True):
+                flash(
+                    _(
+                        "Deletion of wallet in Specter failed, but wallet on node was removed."
+                    ),
+                    "error",
+                )
+            else:
+                flash(_("Deletion of wallet failed."), "error")
+            return redirect(url_for("index"))
         elif action == "rename":
             wallet_name = request.form["newtitle"]
             if not wallet_name:

src/cryptoadvance/specter/templates/node/node_settings.jinja

@@ -41,7 +41,7 @@
 			</div>
 			<span id="datadir-container">
 			<br>
-				{{ _("Bitcoin Core data directory path:") }}<br><input type="text" id="datadir" name="datadir" type="text" value="{{ node.datadir }}" style="margin-bottom: 15px;">
+				{{ _("Bitcoin Core data directory path:") }}<br><input type="text" id="datadir" name="datadir" type="text" value="{{ node.default_datadir }}" style="margin-bottom: 15px;">
 			</span>
 			<br>
 			<div id="rpc_settings">

src/cryptoadvance/specter/user.py

@@ -299,6 +299,9 @@ def check_wallet_manager(self):
                 self.specter.chain,
                 self.device_manager,
                 path=wallets_rpcpath,
+                allow_threading_for_testing=self.specter.config["testing"][
+                    "allow_threading_for_testing"
+                ],
             )
             self._wallet_manager = wallet_manager
         else: