Bugfix: Internal server error when node connection breaks down (#1920)

* BrokenCoreConnectionException added to handle lost RPC + some minor error handling fixes

* clicking redirects to node config if there is no node connection

* handling broken connection when there is no other error superseding (i.e. no wallets)

* fix test results, property device_manager in user, bcce in get_rpc in node, improved rpc property in node

* fix at least one test

* fix for rest test

* doc string for _get_rpc

* fix for node test

* marked test as slow in wallet managers test

* safety checks in node controller

* fix last test, revert deletion of raising exception

Co-authored-by: Kim Neunert <[email protected]>

Author: moneymanolis

src/cryptoadvance/specter/api/rest/resource_psbt.py

@@ -1,13 +1,16 @@
 import logging
+import base64
+from json import dumps
 
 from .base import (
     SecureResource,
     rest_resource,
 )
-from flask import current_app as app, request
+
+from flask import current_app as app, request, Response
 from ...wallet import Wallet
 from ...commands.psbt_creator import PsbtCreator
-
+from ...specter_error import SpecterError
 from .. import auth
 
 logger = logging.getLogger(__name__)
@@ -20,11 +23,16 @@ class ResourcePsbt(SecureResource):
     endpoints = ["/v1alpha/wallets/<wallet_alias>/psbt"]
 
     def get(self, wallet_alias):
-        # ToDo: check whether the user has access to the wallet
         user = auth.current_user()
-        wallet: Wallet = app.specter.user_manager.get_user(
-            user
-        ).wallet_manager.get_by_alias(wallet_alias)
+        wallet_manager = app.specter.user_manager.get_user(user).wallet_manager
+        # Check that the wallet belongs to the user from Basic Auth
+        try:
+            wallet = wallet_manager.get_by_alias(wallet_alias)
+        except SpecterError:
+            error_message = dumps(
+                {"message": "The wallet does not belong to the user in the request."}
+            )
+            return Response(error_message, 403)
         pending_psbts = wallet.pending_psbts_dict()
         return {"result": pending_psbts or {}}
 

src/cryptoadvance/specter/managers/wallet_manager.py

@@ -15,7 +15,7 @@
 from ..liquid.wallet import LWallet
 from ..persistence import delete_folder
 from ..rpc import RpcError, get_default_datadir
-from ..specter_error import SpecterError, handle_exception
+from ..specter_error import SpecterError, SpecterInternalException, handle_exception
 from ..wallet import (  # TODO: `purposes` unused here, but other files rely on this import
     Wallet,
     purposes,
@@ -69,7 +69,7 @@ def update(
         use_threading : for the _update method which is heavily communicating with Bitcoin Core
         """
         if (chain is None and rpc is not None) or (chain is not None and rpc is None):
-            raise Exception(
+            raise SpecterInternalException(
                 f"Chain ({chain}) and rpc ({rpc}) can only be changed with one another"
             )
         if self.is_loading:
@@ -89,7 +89,7 @@ def update(
         else:
             if rpc:
                 logger.error(
-                    f"Prevented Trying to update wallet_Manager with broken {rpc}"
+                    f"Prevented trying to update Wallet Manager with broken {rpc}"
                 )
         # wallets_update_list is something like:
         # {'Specter': {'name': 'Specter', 'alias': 'pacman', ... }, 'another_wallet': { ... } }
@@ -244,7 +244,10 @@ def get_by_alias(self, alias):
         for wallet_name in self.wallets:
             if self.wallets[wallet_name] and self.wallets[wallet_name].alias == alias:
                 return self.wallets[wallet_name]
-        raise SpecterError("Wallet %s does not exist!" % alias)
+        raise SpecterError(
+            "Wallet %s could not be loaded. Are you connected with Bitcoin Core?"
+            % alias
+        )
 
     @property
     def failed_load_wallets(self) -> list:

src/cryptoadvance/specter/node.py

@@ -14,6 +14,7 @@
     autodetect_rpc_confs,
     get_default_datadir,
 )
+from .specter_error import BrokenCoreConnectionException
 
 logger = logging.getLogger(__name__)
 
@@ -74,7 +75,10 @@ def __init__(
         self.manager = manager
         self.proxy_url = manager.proxy_url
         self.only_tor = manager.only_tor
-        self.rpc = self.get_rpc()
+        try:
+            self.rpc = self._get_rpc()
+        except BrokenCoreConnectionException:
+            self.rpc = None
         self._asset_labels = None
         self.check_info()
 
@@ -128,26 +132,29 @@ def json(self):
             "node_type": self.node_type,
         }
 
-    def get_rpc(self):
-        """
-        Checks if config have changed, compares with old rpc
-        and returns new one if necessary
-        """
-        if hasattr(self, "rpc"):
-            rpc = self.rpc
+    def _get_rpc(self):
+        """Checks if configurations have changed, compares with old rpc
+        and returns new one if necessary.
+        Aims to be exception safe, returns None if rpc is not working"""
+        if hasattr(self, "_rpc"):
+            rpc = self._rpc
         else:
             rpc = None
         if self.autodetect:
-            if self.port:
-                rpc_conf_arr = autodetect_rpc_confs(
-                    self.node_type,
-                    datadir=os.path.expanduser(self.datadir),
-                    port=self.port,
-                )
-            else:
-                rpc_conf_arr = autodetect_rpc_confs(
-                    self.node_type, datadir=os.path.expanduser(self.datadir)
-                )
+            try:
+                if self.port:
+                    # autodetect_rpc_confs is trying a RPC call
+                    rpc_conf_arr = autodetect_rpc_confs(
+                        self.node_type,
+                        datadir=os.path.expanduser(self.datadir),
+                        port=self.port,
+                    )
+                else:
+                    rpc_conf_arr = autodetect_rpc_confs(
+                        self.node_type, datadir=os.path.expanduser(self.datadir)
+                    )
+            except BrokenCoreConnectionException:
+                return None
             if len(rpc_conf_arr) > 0:
                 rpc = BitcoinRPC(
                     **rpc_conf_arr[0], proxy_url=self.proxy_url, only_tor=self.only_tor
@@ -171,7 +178,7 @@ def get_rpc(self):
             )
 
         if rpc == None:
-            logger.error(f"connection results to None in get_rpc")
+            logger.error(f"RPC connection is None in get_rpc. Returning None ...")
             return None
         # check if it's liquid
         try:
@@ -184,8 +191,8 @@ def get_rpc(self):
                 return rpc  # The user is failing to configure correctly
             logger.error(rpce)
             return None
-        except ConnectionError as e:
-            logger.error(f"{e} while get_rpc")
+        except BrokenCoreConnectionException as bcce:
+            logger.error(f"{bcce} while get_rpc")
             return None
         except Exception as e:
             logger.exception(e)
@@ -194,7 +201,7 @@ def get_rpc(self):
             return rpc
         else:
             logger.debug(
-                f"connection {rpc} fails test_connection() returning None in get_rpc"
+                f"Connection {rpc} fails test_connection() in get_rpc. Returning None ..."
             )
             return None
 
@@ -239,10 +246,14 @@ def update_rpc(
             self.protocol = protocol
             update_rpc = True
         if update_rpc:
-            self.rpc = self.get_rpc()
-            if self.rpc and self.rpc.test_connection():
-                logger.debug(f"persisting {self} in update_rpc")
-                write_node(self, self.fullpath)
+            try:
+                self.rpc = self._get_rpc()
+                if self.rpc and self.rpc.test_connection():
+                    logger.debug(f"persisting {self} in update_rpc")
+                    write_node(self, self.fullpath)
+            except BrokenCoreConnectionException:
+                self._mark_node_as_broken()
+                return False
         self.check_info()
         return False if not self.rpc else self.rpc.test_connection()
 
@@ -255,7 +266,6 @@ def rename(self, new_name):
 
     def check_info(self):
         self._is_configured = self.rpc is not None
-        self._is_running = False
         if self.rpc is not None and self.rpc.test_connection():
             try:
                 res = [
@@ -289,12 +299,8 @@ def check_info(self):
                     self.utxorescanwallet = None
                 self._network_parameters = get_network(self.chain)
                 self._is_running = True
-            except Exception as e:
-                self._info = {"chain": None}
-                self._network_info = {"subversion": "", "version": 999999}
-                self._network_parameters = get_network("main")
-                logger.error(f"connection {self.rpc} could not suceed check_info")
-                logger.exception("Exception %s while check_info()" % e)
+            except BrokenCoreConnectionException:
+                self._mark_node_as_broken()
         else:
             if self.rpc is None:
                 logger.warning(f"connection of {self} is None in check_info")
@@ -315,24 +321,20 @@ def check_info(self):
                     )
                 except Exception as e:
                     logger.exception(e)
-            self._info = {"chain": None}
-            self._network_info = {"subversion": "", "version": 999999}
-
-        if not self._is_running:
-            self._info["chain"] = None
+            self._mark_node_as_broken()
 
     def test_rpc(self):
         """tests the rpc-connection and returns a dict which helps
         to derive what might be wrong with the config
         ToDo: list an example here.
         """
-        rpc = self.get_rpc()
+        rpc = self._get_rpc()
         if rpc is None:
             return {
                 "out": "",
                 "err": _("Connection to node failed"),
                 "code": -1,
-                "tests": {},
+                "tests": {"connectable": False},
             }
         r = {}
         r["tests"] = {"connectable": False}
@@ -356,15 +358,14 @@ def test_rpc(self):
                 r["err"] = "Wallets disabled"
 
             r["out"] = json.dumps(rpc.getblockchaininfo(), indent=4)
-        except ConnectionError as e:
-            logger.info("Caught an ConnectionError while test_rpc: %s", e)
-
+        except BrokenCoreConnectionException as bcce:
+            logger.info(f"Caught {bcce} while test_rpc")
             r["tests"]["connectable"] = False
             r["err"] = _("Failed to connect!")
             r["code"] = -1
         except RpcError as rpce:
             logger.info(
-                f"Caught an RpcError while test_rpc status_code: {rpce.status_code} error_code:{rpce.error_code}"
+                f"Caught an RpcError while test_rpc status_code: {rpce.status_code} error_code: {rpce.error_code}"
             )
             r["tests"]["connectable"] = True
             r["code"] = rpc.r.status_code
@@ -374,7 +375,7 @@ def test_rpc(self):
             else:
                 r["err"] = str(rpce.status_code)
         except Exception as e:
-            logger.error(
+            logger.exception(
                 "Caught an exception of type {} while test_rpc: {}".format(
                     type(e), str(e)
                 )
@@ -388,6 +389,16 @@ def test_rpc(self):
                 r["code"] = -1
         return r
 
+    def _mark_node_as_broken(self):
+        self._info = {"chain": None}
+        self._network_info = {"subversion": "", "version": 999999}
+        self._network_parameters = get_network("main")
+        logger.debug(
+            f"Node is not running, no RPC connection, check_info didn't succeed, setting RPC attribute to None ..."
+        )
+        self._info["chain"] = None
+        self.rpc = None
+
     def abortrescanutxo(self):
         """use this to abort a rescan as it stores some state while rescanning"""
         self.rpc.scantxoutset("abort", [])
@@ -409,7 +420,11 @@ def is_liquid(self):
 
     @property
     def is_running(self):
-        return self._is_running
+        if self._network_info["version"] == 999999:
+            logger.debug(f"Node is not running")
+            return False
+        else:
+            return True
 
     @property
     def is_configured(self):
@@ -478,10 +493,12 @@ def is_liquid(self):
 
     @property
     def rpc(self):
+        """Returns None if rpc is broken"""
         if not hasattr(self, "_rpc"):
-            return None
-        else:
-            return self._rpc
+            self._rpc = self._get_rpc()
+        elif self._rpc is None:
+            self._rpc = self._get_rpc()
+        return self._rpc
 
     @property
     def node_type(self):

src/cryptoadvance/specter/process_controller/node_controller.py

@@ -19,7 +19,11 @@
 
 from ..helpers import load_jsons
 from ..rpc import BitcoinRPC, RpcError
-from ..specter_error import ExtProcTimeoutException, SpecterError
+from ..specter_error import (
+    ExtProcTimeoutException,
+    SpecterError,
+    BrokenCoreConnectionException,
+)
 from ..util.shell import get_last_lines_from_file, which
 
 logger = logging.getLogger(__name__)
@@ -272,11 +276,19 @@ def testcoin_faucet(self, address, amount=20, confirm_payment=True):
     def check_node(rpcconn, raise_exception=False):
         """returns true if bitcoind is running on that address/port"""
         if raise_exception:
-            rpcconn.get_rpc()  # that call will also check the connection
-            return True
+            rpc = rpcconn.get_rpc()  # that call will also check the connection
+            if (
+                rpc
+            ):  # if-else is just in case, ideally, rpc should not be returned as None
+                return True
+            return False
         try:
-            rpcconn.get_rpc()  # that call will also check the connection
-            return True
+            rpc = rpcconn.get_rpc()  # that call will also check the connection
+            if (
+                rpc
+            ):  # if-else is just in case, ideally, rpc should not be returned as None
+                return True
+            return False
         except RpcError as e:
             # E.g. "Loading Index ..." #ToDo: check it here
             return False
@@ -288,10 +300,14 @@ def check_node(rpcconn, raise_exception=False):
             return False
         except NewConnectionError as e:
             return False
+        except BrokenCoreConnectionException:
+            return False
         except Exception as e:
             # We should avoid this:
             # If you see it in the logs, catch that new exception above
-            logger.error("Unexpected Exception, THIS SHOULD NOT HAPPEN " + str(type(e)))
+            logger.exception(
+                "Unexpected Exception, THIS SHOULD NOT HAPPEN " + str(type(e))
+            )
             logger.debug(f"could not reach bitcoind - message returned: {e}")
             return False
 

src/cryptoadvance/specter/rpc.py

@@ -9,7 +9,9 @@
 import urllib3
 
 from .helpers import is_ip_private
-from .specter_error import SpecterError, handle_exception
+from .specter_error import SpecterError, handle_exception, BrokenCoreConnectionException
+from urllib3.exceptions import NewConnectionError
+from requests.exceptions import ConnectionError
 
 logger = logging.getLogger(__name__)
 
@@ -366,6 +368,9 @@ def multi(self, calls: list, **kwargs):
             r = self.session.post(
                 url, data=json.dumps(payload), headers=headers, timeout=timeout
             )
+        except (ConnectionError, NewConnectionError, ConnectionRefusedError) as ce:
+            raise BrokenCoreConnectionException()
+
         except (requests.exceptions.Timeout, urllib3.exceptions.ReadTimeoutError) as to:
             # Timeout is effectively one of the two:
             # ConnectTimeout: The request timed out while trying to connect to the remote server