From 05649861c8155c890e18e4d5da93552ef94587b2 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Wed, 23 Mar 2022 03:58:33 +0100 Subject: [PATCH] qt gui: more resilient startup: catch more exceptions, better fallback fixes https://github.com/spesmilo/electrum/issues/7447 Consider this trace for 4.2.0: ``` Traceback (most recent call last): File "electrum/gui/qt/__init__.py", line 332, in start_new_window File "electrum/gui/qt/__init__.py", line 363, in _start_wizard_to_select_or_create_wallet File "electrum/gui/qt/installwizard.py", line 302, in select_storage File "electrum/util.py", line 504, in get_new_wallet_name PermissionError: [Errno 1] Operation not permitted: '/Users/admin/Documents/Peach/MS' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "electrum/gui/qt/__init__.py", line 426, in main File "electrum/gui/qt/__init__.py", line 307, in wrapper File "electrum/gui/qt/__init__.py", line 349, in start_new_window File "electrum/util.py", line 504, in get_new_wallet_name PermissionError: [Errno 1] Operation not permitted: '/Users/admin/Documents/Peach/MS' ``` Note that `get_new_wallet_name` (os.listdir) can raise OSError, and we were calling that on the main entrypoint codepath without exception-handling. We were also calling it in the fallback codepath without exception-handling. i.e. the GUI errored out on every startup for affected users, and without CLI usage it was not possible to recover. --- electrum/gui/qt/__init__.py | 52 +++++++++++++++++++++++--------- electrum/gui/qt/installwizard.py | 6 ++-- electrum/gui/qt/main_window.py | 12 ++++++-- electrum/simple_config.py | 14 +++++---- electrum/util.py | 3 ++ 5 files changed, 60 insertions(+), 27 deletions(-) diff --git a/electrum/gui/qt/__init__.py b/electrum/gui/qt/__init__.py index 80859d90b..894a4abac 100644 --- a/electrum/gui/qt/__init__.py +++ b/electrum/gui/qt/__init__.py @@ -320,21 +320,33 @@ class ElectrumGui(BaseElectrumGui, Logger): return wrapper @count_wizards_in_progress - def start_new_window(self, path, uri, *, app_is_starting=False) -> Optional[ElectrumWindow]: + def start_new_window( + self, + path, + uri: Optional[str], + *, + app_is_starting: bool = False, + force_wizard: bool = False, + ) -> Optional[ElectrumWindow]: '''Raises the window for the wallet if it is open. Otherwise opens the wallet and creates a new window for it''' wallet = None - try: - wallet = self.daemon.load_wallet(path, None) - except Exception as e: - self.logger.exception('') - custom_message_box(icon=QMessageBox.Warning, - parent=None, - title=_('Error'), - text=_('Cannot load wallet') + ' (1):\n' + repr(e)) - # if app is starting, still let wizard appear - if not app_is_starting: - return + # Try to open with daemon first. If this succeeds, there won't be a wizard at all + # (the wallet main window will appear directly). + if not force_wizard: + try: + wallet = self.daemon.load_wallet(path, None) + except Exception as e: + self.logger.exception('') + custom_message_box(icon=QMessageBox.Warning, + parent=None, + title=_('Error'), + text=_('Cannot load wallet') + ' (1):\n' + repr(e)) + # if app is starting, still let wizard appear + if not app_is_starting: + return + # Open a wizard window. This lets the user e.g. enter a password, or select + # a different wallet. try: if not wallet: wallet = self._start_wizard_to_select_or_create_wallet(path) @@ -353,9 +365,19 @@ class ElectrumGui(BaseElectrumGui, Logger): title=_('Error'), text=_('Cannot load wallet') + '(2) :\n' + repr(e)) if app_is_starting: - wallet_dir = os.path.dirname(path) - path = os.path.join(wallet_dir, get_new_wallet_name(wallet_dir)) - self.start_new_window(path, uri) + # If we raise in this context, there are no more fallbacks, we will shut down. + # Worst case scenario, we might have gotten here without user interaction, + # in which case, if we raise now without user interaction, the same sequence of + # events is likely to repeat when the user restarts the process. + # So we play it safe: clear path, clear uri, force a wizard to appear. + try: + wallet_dir = os.path.dirname(path) + filename = get_new_wallet_name(wallet_dir) + except OSError: + path = self.config.get_fallback_wallet_path() + else: + path = os.path.join(wallet_dir, filename) + self.start_new_window(path, uri=None, force_wizard=True) return if uri: window.pay_to_URI(uri) diff --git a/electrum/gui/qt/installwizard.py b/electrum/gui/qt/installwizard.py index c20ee76f3..3d4541ff6 100644 --- a/electrum/gui/qt/installwizard.py +++ b/electrum/gui/qt/installwizard.py @@ -202,6 +202,8 @@ class InstallWizard(QDialog, MessageBoxMixin, BaseWizard): self.refresh_gui() # Need for QT on MacOSX. Lame. def select_storage(self, path, get_wallet_from_daemon) -> Tuple[str, Optional[WalletStorage]]: + if os.path.isdir(path): + raise Exception("wallet path cannot point to a directory") vbox = QVBoxLayout() hbox = QHBoxLayout() @@ -297,9 +299,7 @@ class InstallWizard(QDialog, MessageBoxMixin, BaseWizard): button.clicked.connect(on_choose) button_create_new.clicked.connect( - partial( - name_e.setText, - get_new_wallet_name(wallet_folder))) + lambda: name_e.setText(get_new_wallet_name(wallet_folder))) # FIXME get_new_wallet_name might raise name_e.textChanged.connect(on_filename) name_e.setText(os.path.basename(path)) diff --git a/electrum/gui/qt/main_window.py b/electrum/gui/qt/main_window.py index 78e2106fb..ab1dcc661 100644 --- a/electrum/gui/qt/main_window.py +++ b/electrum/gui/qt/main_window.py @@ -676,9 +676,15 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger): except FileNotFoundError as e: self.show_error(str(e)) return - filename = get_new_wallet_name(wallet_folder) - full_path = os.path.join(wallet_folder, filename) - self.gui_object.start_new_window(full_path, None) + try: + filename = get_new_wallet_name(wallet_folder) + except OSError as e: + self.logger.exception("") + self.show_error(repr(e)) + path = self.config.get_fallback_wallet_path() + else: + path = os.path.join(wallet_folder, filename) + self.gui_object.start_new_window(path, uri=None, force_wizard=True) def init_menubar(self): menubar = QMenuBar() diff --git a/electrum/simple_config.py b/electrum/simple_config.py index 8320b63f4..91cc6d016 100644 --- a/electrum/simple_config.py +++ b/electrum/simple_config.py @@ -296,12 +296,7 @@ class SimpleConfig(Logger): if path and os.path.exists(path): return path - # default path - util.assert_datadir_available(self.path) - dirpath = os.path.join(self.path, "wallets") - make_dir(dirpath, allow_symlink=False) - - new_path = os.path.join(self.path, "wallets", "default_wallet") + new_path = self.get_fallback_wallet_path() # default path in pre 1.9 versions old_path = os.path.join(self.path, "electrum.dat") @@ -310,6 +305,13 @@ class SimpleConfig(Logger): return new_path + def get_fallback_wallet_path(self): + util.assert_datadir_available(self.path) + dirpath = os.path.join(self.path, "wallets") + make_dir(dirpath, allow_symlink=False) + path = os.path.join(self.path, "wallets", "default_wallet") + return path + def remove_from_recently_open(self, filename): recent = self.get('recently_open', []) if filename in recent: diff --git a/electrum/util.py b/electrum/util.py index 30bc0cb45..ee960c82e 100644 --- a/electrum/util.py +++ b/electrum/util.py @@ -498,6 +498,9 @@ def standardize_path(path): def get_new_wallet_name(wallet_folder: str) -> str: + """Returns a file basename for a new wallet to be used. + Can raise OSError. + """ i = 1 while True: filename = "wallet_%d" % i