From 68fb996d200a9a48f93891bb0bdd038c48d4a5ab Mon Sep 17 00:00:00 2001 From: SomberNight Date: Thu, 11 May 2023 13:48:54 +0000 Subject: [PATCH] wallet_db version 52: break non-homogeneous multisig wallets - case 1: in version 4.4.1, 4.4.2, the qml GUI wizard allowed creating multisig wallets with an old_mpk as cosigner. - case 2: in version 4.4.0, 4.4.1, 4.4.2, the qml GUI wizard allowed creating multisig wallets with mixed xpub/Ypub/Zpub. The corresponding missing input validation was a bug in the wizard, it was unintended behaviour. Validation was added in d2cf21fc2bcf79f07b7e41178cd3e4ca9e3d9f68. Note however that there might be users who created such wallet files. Re case 1 wallet files: there is no version of Electrum that allows spending from such a wallet. Coins received at addresses are not burned, however it is technically challenging to spend them. (unless the multisig can spend without needing the old_mpk cosigner in the quorum). Re case 2 wallet files: it is possible to create a corresponding spending wallet for such a multisig, however it is a bit tricky. The script type for the addresses in such a heterogeneous xpub wallet is based on the xpub_type of the first keystore. So e.g. given a wallet file [Yprv1, Zpub2] it will have sh(wsh()) scripts, and the cosigner should create a wallet file [Ypub1, Zprv2] (same order). Technically case 2 wallet files could be "fixed" automatically by converting the xpub types as part of a wallet_db upgrade. However if the wallet files also contain seeds, those cannot be converted ("standard" vs "segwit" electrum seed). Case 1 wallet files are not possible to "fix" automatically as the cosigner using the old_mpk is not bip32 based. It is unclear if there are *any* users out there affected by this. I suspect for case 1 it is very likely there are none (not many people have pre-2.0 electrum seeds which were never supported as part of a multisig who would also now try to create a multisig using them); for case 2 however there might be. This commit breaks both case 1 and case 2 wallets: these wallet files can no longer be opened in new Electrum, an error message is shown and the crash reporter opens. If any potential users opt to send crash reports, at least we will know they exist and can help them recover. --- electrum/gui/qml/components/main.qml | 2 +- electrum/gui/qml/qedaemon.py | 2 +- electrum/gui/qml/qewalletdb.py | 60 ++++++++++++++------------ electrum/gui/qt/__init__.py | 10 ++++- electrum/util.py | 5 ++- electrum/wallet.py | 6 +++ electrum/wallet_db.py | 64 ++++++++++++++++++++++++++-- 7 files changed, 113 insertions(+), 36 deletions(-) diff --git a/electrum/gui/qml/components/main.qml b/electrum/gui/qml/components/main.qml index 7ecd533bc..e49f88ec9 100644 --- a/electrum/gui/qml/components/main.qml +++ b/electrum/gui/qml/components/main.qml @@ -495,7 +495,7 @@ ApplicationWindow } function onWalletOpenError(error) { console.log('wallet open error') - var dialog = app.messageDialog.createObject(app, {'text': error}) + var dialog = app.messageDialog.createObject(app, { title: qsTr('Error'), 'text': error }) dialog.open() } function onAuthRequired(method, authMessage) { diff --git a/electrum/gui/qml/qedaemon.py b/electrum/gui/qml/qedaemon.py index 6f4a50b5a..037d40d57 100644 --- a/electrum/gui/qml/qedaemon.py +++ b/electrum/gui/qml/qedaemon.py @@ -223,7 +223,7 @@ class QEDaemon(AuthMixin, QObject): self._backendWalletLoaded.emit(local_password) except WalletFileException as e: - self._logger.error(str(e)) + self._logger.error(f"load_wallet_task errored opening wallet: {e!r}") self.walletOpenError.emit(str(e)) finally: self._loading = False diff --git a/electrum/gui/qml/qewalletdb.py b/electrum/gui/qml/qewalletdb.py index 45a59947e..7b49970d2 100644 --- a/electrum/gui/qml/qewalletdb.py +++ b/electrum/gui/qml/qewalletdb.py @@ -8,7 +8,7 @@ from electrum.storage import WalletStorage, StorageEncryptionVersion from electrum.wallet_db import WalletDB from electrum.wallet import Wallet from electrum.bip32 import normalize_bip32_derivation, xpub_type -from electrum.util import InvalidPassword, WalletFileException +from electrum.util import InvalidPassword, WalletFileException, send_exception_to_crash_reporter from electrum import keystore if TYPE_CHECKING: @@ -120,9 +120,16 @@ class QEWalletDB(QObject): @pyqtSlot() def verify(self): - self.load_storage() - if self._storage: - self.load_db() + try: + self._load_storage() + if self._storage: + self._load_db() + except WalletFileException as e: + self._logger.error(f"verify errored: {repr(e)}") + self._storage = None + self.walletOpenProblem.emit(str(e)) + if e.should_report_crash: + send_exception_to_crash_reporter(e) @pyqtSlot() def doSplit(self): @@ -134,7 +141,8 @@ class QEWalletDB(QObject): self.splitFinished.emit() - def load_storage(self): + def _load_storage(self): + """can raise WalletFileException""" self._storage = WalletStorage(self._path) if not self._storage.file_exists(): self._logger.warning('file does not exist') @@ -170,27 +178,23 @@ class QEWalletDB(QObject): if not self._storage.is_past_initial_decryption(): self._storage = None - def load_db(self): + def _load_db(self): + """can raise WalletFileException""" # needs storage accessible - try: - self._db = WalletDB(self._storage.read(), manual_upgrades=True) - if self._db.requires_split(): - self._logger.warning('wallet requires split') - self._requiresSplit = True - self.requiresSplitChanged.emit() - return - if self._db.get_action(): - self._logger.warning('action pending. QML version doesn\'t support continuation of wizard') - return - - if self._db.requires_upgrade(): - self._logger.warning('wallet requires upgrade, upgrading') - self._db.upgrade() - self._db.write(self._storage) - - self._ready = True - self.readyChanged.emit() - except WalletFileException as e: - self._logger.error(f'{repr(e)}') - self._storage = None - self.walletOpenProblem.emit(str(e)) + self._db = WalletDB(self._storage.read(), manual_upgrades=True) + if self._db.requires_split(): + self._logger.warning('wallet requires split') + self._requiresSplit = True + self.requiresSplitChanged.emit() + return + if self._db.get_action(): + self._logger.warning('action pending. QML version doesn\'t support continuation of wizard') + return + + if self._db.requires_upgrade(): + self._logger.warning('wallet requires upgrade, upgrading') + self._db.upgrade() + self._db.write(self._storage) + + self._ready = True + self.readyChanged.emit() diff --git a/electrum/gui/qt/__init__.py b/electrum/gui/qt/__init__.py index 326d70bd7..e8d82d0ab 100644 --- a/electrum/gui/qt/__init__.py +++ b/electrum/gui/qt/__init__.py @@ -342,10 +342,13 @@ class ElectrumGui(BaseElectrumGui, Logger): wallet = self.daemon.load_wallet(path, None) except Exception as e: self.logger.exception('') + err_text = str(e) if isinstance(e, WalletFileException) else repr(e) custom_message_box(icon=QMessageBox.Warning, parent=None, title=_('Error'), - text=_('Cannot load wallet') + ' (1):\n' + repr(e)) + text=_('Cannot load wallet') + ' (1):\n' + err_text) + if isinstance(e, WalletFileException) and e.should_report_crash: + send_exception_to_crash_reporter(e) # if app is starting, still let wizard appear if not app_is_starting: return @@ -364,10 +367,13 @@ class ElectrumGui(BaseElectrumGui, Logger): window = self._create_window_for_wallet(wallet) except Exception as e: self.logger.exception('') + err_text = str(e) if isinstance(e, WalletFileException) else repr(e) custom_message_box(icon=QMessageBox.Warning, parent=None, title=_('Error'), - text=_('Cannot load wallet') + '(2) :\n' + repr(e)) + text=_('Cannot load wallet') + '(2) :\n' + err_text) + if isinstance(e, WalletFileException) and e.should_report_crash: + send_exception_to_crash_reporter(e) if app_is_starting: # 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, diff --git a/electrum/util.py b/electrum/util.py index 05131e812..c091fa196 100644 --- a/electrum/util.py +++ b/electrum/util.py @@ -187,7 +187,10 @@ class FileExportFailed(Exception): return _("Failed to export to file.") + "\n" + self.message -class WalletFileException(Exception): pass +class WalletFileException(Exception): + def __init__(self, message='', *, should_report_crash: bool = False): + Exception.__init__(self, message) + self.should_report_crash = should_report_crash class BitcoinException(Exception): pass diff --git a/electrum/wallet.py b/electrum/wallet.py index 567bb4fce..1ac27628d 100644 --- a/electrum/wallet.py +++ b/electrum/wallet.py @@ -3532,6 +3532,12 @@ class Multisig_Wallet(Deterministic_Wallet): self.wallet_type = db.get('wallet_type') self.m, self.n = multisig_type(self.wallet_type) Deterministic_Wallet.__init__(self, db, storage, config=config) + # sanity checks + for ks in self.get_keystores(): + if not isinstance(ks, keystore.Xpub): + raise Exception(f"unexpected keystore type={type(ks)} in multisig") + if bip32.xpub_type(self.keystore.xpub) != bip32.xpub_type(ks.xpub): + raise Exception(f"multisig wallet needs to have homogeneous xpub types") def get_public_keys(self, address): return [pk.hex() for pk in self.get_public_keys_with_deriv_info(address)] diff --git a/electrum/wallet_db.py b/electrum/wallet_db.py index fd17f21c7..1336b609e 100644 --- a/electrum/wallet_db.py +++ b/electrum/wallet_db.py @@ -57,7 +57,7 @@ if TYPE_CHECKING: OLD_SEED_VERSION = 4 # electrum versions < 2.0 NEW_SEED_VERSION = 11 # electrum versions >= 2.0 -FINAL_SEED_VERSION = 51 # electrum >= 2.7 will set this to prevent +FINAL_SEED_VERSION = 52 # electrum >= 2.7 will set this to prevent # old versions from overwriting new format @@ -81,6 +81,12 @@ class DBMetadata(StoredObject): return f"using {ver}, on {date_str}" +# note: subclassing WalletFileException for some specific cases +# allows the crash reporter to distinguish them and open +# separate tracking issues +class WalletFileExceptionVersion51(WalletFileException): pass + + class WalletDB(JsonDB): def __init__(self, raw, *, manual_upgrades: bool): @@ -220,6 +226,7 @@ class WalletDB(JsonDB): self._convert_version_49() self._convert_version_50() self._convert_version_51() + self._convert_version_52() self.put('seed_version', FINAL_SEED_VERSION) # just to be sure self._after_upgrade_tasks() @@ -1016,6 +1023,38 @@ class WalletDB(JsonDB): item['payment_hash'] = payment_hash self.data['seed_version'] = 51 + def _detect_insane_version_51(self) -> int: + """Returns 0 if file okay, + error code 1: multisig wallet has old_mpk + error code 2: multisig wallet has mixed Ypub/Zpub + """ + assert self.get('seed_version') == 51 + xpub_type = None + for ks_name in ['x{}/'.format(i) for i in range(1, 16)]: # having any such field <=> multisig wallet + ks = self.data.get(ks_name, None) + if ks is None: continue + ks_type = ks.get('type') + if ks_type == "old": + return 1 # error + assert ks_type in ("bip32", "hardware"), f"unexpected {ks_type=}" + xpub = ks.get('xpub') or None + assert xpub is not None + assert isinstance(xpub, str) + if xpub_type is None: # first iter + xpub_type = xpub[0:4] + if xpub[0:4] != xpub_type: + return 2 # error + # looks okay + return 0 + + def _convert_version_52(self): + if not self._is_upgrade_method_needed(51, 51): + return + if (error_code := self._detect_insane_version_51()) != 0: + # should not get here; get_seed_version should have caught this + raise Exception(f'unsupported wallet file: version_51 with error {error_code}') + self.data['seed_version'] = 52 + def _convert_imported(self): if not self._is_upgrade_method_needed(0, 13): return @@ -1071,9 +1110,11 @@ class WalletDB(JsonDB): raise WalletFileException('This version of Electrum is too old to open this wallet.\n' '(highest supported storage version: {}, version of this file: {})' .format(FINAL_SEED_VERSION, seed_version)) - if seed_version==14 and self.get('seed_type') == 'segwit': + if seed_version == 14 and self.get('seed_type') == 'segwit': self._raise_unsupported_version(seed_version) - if seed_version >=12: + if seed_version == 51 and self._detect_insane_version_51(): + self._raise_unsupported_version(seed_version) + if seed_version >= 12: return seed_version if seed_version not in [OLD_SEED_VERSION, NEW_SEED_VERSION]: self._raise_unsupported_version(seed_version) @@ -1092,6 +1133,23 @@ class WalletDB(JsonDB): else: # creation was complete if electrum was run from source msg += "\nPlease open this file with Electrum 1.9.8, and move your coins to a new wallet." + if seed_version == 51: + error_code = self._detect_insane_version_51() + assert error_code != 0 + msg += f" ({error_code=})" + if error_code == 1: + msg += "\nThis is a multisig wallet containing an old_mpk (pre-bip32 master public key)." + msg += "\nPlease contact us to help recover it by opening an issue on GitHub." + elif error_code == 2: + msg += ("\nThis is a multisig wallet containing mixed xpub/Ypub/Zpub." + "\nThe script type is determined by the type of the first keystore." + "\nTo recover, you should re-create the wallet with matching type " + "(converted if needed) master keys." + "\nOr you can contact us to help recover it by opening an issue on GitHub.") + else: + raise Exception(f"unexpected {error_code=}") + raise WalletFileExceptionVersion51(msg, should_report_crash=True) + # generic exception raise WalletFileException(msg) def _add_db_creation_metadata(self):