qt: multisig checks with hardware cosigners

electrum/gui/qt/wizard/server_connect.py

@@ -1,12 +1,10 @@
 from typing import TYPE_CHECKING
 
-from PyQt5.QtWidgets import QApplication
-
 from electrum.i18n import _
 from .wizard import QEAbstractWizard, WizardComponent
 from electrum.wizard import ServerConnectWizard
-from ..network_dialog import ProxyWidget, ServerWidget
-from ..util import ChoicesLayout
+from electrum.gui.qt.network_dialog import ProxyWidget, ServerWidget
+from electrum.gui.qt.util import ChoiceWidget
 
 if TYPE_CHECKING:
     from electrum.simple_config import SimpleConfig
@@ -19,8 +17,7 @@ class QEServerConnectWizard(ServerConnectWizard, QEAbstractWizard):
 
     def __init__(self, config: 'SimpleConfig', app: 'QElectrumApplication', plugins: 'Plugins', daemon: 'Daemon', parent=None):
         ServerConnectWizard.__init__(self, daemon)
-        QEAbstractWizard.__init__(self, config, app, plugins, daemon)
-        self._daemon = daemon
+        QEAbstractWizard.__init__(self, config, app)
 
         # attach view names
         self.navmap_merge({
@@ -40,14 +37,16 @@ class WCAutoConnect(WizardComponent):
                   "hardware. In most cases you simply want to let Electrum "
                   "pick one at random.  However if you prefer feel free to "
                   "select a server manually.")
-        choices = [_("Auto connect"), _("Select server manually")]
-        self.clayout = ChoicesLayout(message, choices, on_clicked=self.on_updated)
-        self.layout().addLayout(self.clayout.layout())
+        choices = [('autoconnect', _("Auto connect")),
+                   ('select', _("Select server manually"))]
+        self.choice_w = ChoiceWidget(message=message, choices=choices)
+        self.choice_w.itemSelected.connect(self.on_updated)
+        self.layout().addWidget(self.choice_w)
         self.layout().addStretch(1)
         self._valid = True
 
     def apply(self):
-        r = self.clayout.selected_index()
+        r = self.choice_w.selected_index
         self.wizard_data['autoconnect'] = (r == 0)
         # if r == 1:
         #     nlayout = NetworkChoiceLayout(network, self.config, wizard=True)
@@ -63,14 +62,15 @@ class WCProxyAsk(WizardComponent):
     def __init__(self, parent, wizard):
         WizardComponent.__init__(self, parent, wizard, title=_("Proxy"))
         message = _("Do you use a local proxy service such as TOR to reach the internet?")
-        choices = [_("Yes"), _("No")]
-        self.clayout = ChoicesLayout(message, choices)
-        self.layout().addLayout(self.clayout.layout())
+        choices = [('yes', _("Yes")),
+                   ('no', _("No"))]
+        self.choice_w = ChoiceWidget(message=message, choices=choices)
+        self.layout().addWidget(self.choice_w)
         self.layout().addStretch(1)
         self._valid = True
 
     def apply(self):
-        r = self.clayout.selected_index()
+        r = self.choice_w.selected_index
         self.wizard_data['want_proxy'] = (r == 0)
 
 

electrum/gui/qt/wizard/wallet.py

@@ -51,8 +51,8 @@ class QENewWalletWizard(NewWalletWizard, QEAbstractWizard):
 
     def __init__(self, config: 'SimpleConfig', app: 'QElectrumApplication', plugins: 'Plugins', daemon: Daemon, path, parent=None):
         NewWalletWizard.__init__(self, daemon, plugins)
-        QEAbstractWizard.__init__(self, config, app, plugins, daemon)
-        self._daemon = daemon  # TODO: dedupe
+        QEAbstractWizard.__init__(self, config, app)
+
         self._path = path
 
         # attach gui classes to views
@@ -408,7 +408,7 @@ class WCEnterExt(WizardComponent):
             self.valid = False
             return
 
-        cosigner_data = self._current_cosigner(self.wizard_data)
+        cosigner_data = self.wizard.current_cosigner(self.wizard_data)
 
         if self.wizard_data['wallet_type'] == 'multisig':
             if 'seed_variant' in cosigner_data and cosigner_data['seed_variant'] in ['bip39', 'slip39']:
@@ -429,7 +429,7 @@ class WCEnterExt(WizardComponent):
             self.valid = True
 
     def apply(self):
-        cosigner_data = self._current_cosigner(self.wizard_data)
+        cosigner_data = self.wizard.current_cosigner(self.wizard_data)
         cosigner_data['seed_extra_words'] = self.ext_edit.text()
 
 
@@ -521,7 +521,7 @@ class WCHaveSeed(WizardComponent):
             self.valid = seed_valid
 
     def apply(self):
-        cosigner_data = self._current_cosigner(self.wizard_data)
+        cosigner_data = self.wizard.current_cosigner(self.wizard_data)
 
         cosigner_data['seed'] = self.slayout.get_seed()
         cosigner_data['seed_variant'] = self.slayout.seed_type
@@ -619,28 +619,19 @@ class WCScriptAndDerivation(WizardComponent):
     def validate(self):
         self.apply()
 
-        cosigner_data = self._current_cosigner(self.wizard_data)
+        cosigner_data = self.wizard.current_cosigner(self.wizard_data)
         derivation_valid = is_bip32_derivation(cosigner_data['derivation_path'])
 
-        # TODO: refactor to wizard.current_cosigner_is_hardware
-        cosigner_is_hardware = cosigner_data == self.wizard_data and self.wizard_data['keystore_type'] == 'hardware'
-        if 'cosigner_keystore_type' in self.wizard_data and self.wizard_data['cosigner_keystore_type'] == 'hardware':
-            cosigner_is_hardware = True
-
-        if self.wizard.is_multisig(self.wizard_data) and derivation_valid and not cosigner_is_hardware:
-            if self.wizard.has_duplicate_masterkeys(self.wizard_data):
-                self._logger.debug('Duplicate master keys!')
-                # TODO: user feedback
-                derivation_valid = False
-            elif self.wizard.has_heterogeneous_masterkeys(self.wizard_data):
-                self._logger.debug('Heterogenous master keys!')
+        if derivation_valid:
+            valid, error = self.wizard.check_multisig_constraints(self.wizard_data)
+            if not valid:
                 # TODO: user feedback
-                derivation_valid = False
+                self.logger.error(error)
 
-        self.valid = derivation_valid
+        self.valid = valid
 
     def apply(self):
-        cosigner_data = self._current_cosigner(self.wizard_data)
+        cosigner_data = self.wizard.current_cosigner(self.wizard_data)
         cosigner_data['script_type'] = self.choice_w.selected_item[0]
         cosigner_data['derivation_path'] = str(self.derivation_path_edit.text())
 
@@ -745,7 +736,7 @@ class WCHaveMasterKey(WizardComponent):
 
     def apply(self):
         text = self.slayout.get_text()
-        cosigner_data = self._current_cosigner(self.wizard_data)
+        cosigner_data = self.wizard.current_cosigner(self.wizard_data)
         cosigner_data['master_key'] = text
 
 

electrum/gui/qt/wizard/wizard.py

@@ -22,11 +22,10 @@ class QEAbstractWizard(QDialog, MessageBoxMixin):
     _logger = get_logger(__name__)
 
     # def __init__(self, config: 'SimpleConfig', app: QApplication, plugins: 'Plugins', *, gui_object: 'ElectrumGui'):
-    def __init__(self, config: 'SimpleConfig', app: 'QElectrumApplication', plugins: 'Plugins', daemon: 'Daemon'):
+    def __init__(self, config: 'SimpleConfig', app: 'QElectrumApplication'):
         QDialog.__init__(self, None)
         self.app = app
         self.config = config
-        # self.plugins = plugins
 
         # compat
         self.gui_thread = threading.current_thread()
@@ -266,12 +265,3 @@ class WizardComponent(QWidget):
     @pyqtSlot()
     def on_updated(self, *args):
         self.updated.emit(self)
-
-    # returns (sub)dict of current cosigner (or root if first)
-    # TODO: maybe just always expose self.cosigner_data in wizardcomponent so we can avoid this call
-    def _current_cosigner(self, wizard_data):
-        wdata = wizard_data
-        if wizard_data['wallet_type'] == 'multisig' and 'multisig_current_cosigner' in wizard_data:
-            cosigner = wizard_data['multisig_current_cosigner']
-            wdata = wizard_data['multisig_cosigner_data'][str(cosigner)]
-        return wdata

electrum/plugins/trezor/qt.py

@@ -5,20 +5,23 @@ from PyQt5.QtCore import Qt, QEventLoop, pyqtSignal
 from PyQt5.QtWidgets import (QVBoxLayout, QLabel, QGridLayout, QPushButton,
                              QHBoxLayout, QButtonGroup, QGroupBox, QDialog,
                              QLineEdit, QRadioButton, QCheckBox, QWidget,
-                             QMessageBox, QFileDialog, QSlider, QTabWidget)
+                             QMessageBox, QSlider, QTabWidget)
 
-from electrum.gui.qt.util import (WindowModalDialog, WWLabel, Buttons, CancelButton,
-                                  OkButton, CloseButton, PasswordLineEdit, getOpenFileName, ChoicesLayout)
 from electrum.i18n import _
+from electrum.logging import Logger
 from electrum.plugin import hook
 
-from ..hw_wallet.qt import QtHandlerBase, QtPluginBase
-from ..hw_wallet.plugin import only_hook_if_libraries_available
+from electrum.plugins.hw_wallet.qt import QtHandlerBase, QtPluginBase
+from electrum.plugins.hw_wallet.plugin import only_hook_if_libraries_available
+
+from electrum.gui.qt.util import (WindowModalDialog, WWLabel, Buttons, CancelButton,
+                                  OkButton, CloseButton, PasswordLineEdit, getOpenFileName, ChoiceWidget)
+from electrum.gui.qt.wizard.wallet import WCScriptAndDerivation
+from electrum.gui.qt.wizard.wizard import WizardComponent
+
 from .trezor import (TrezorPlugin, TIM_NEW, TIM_RECOVER, TrezorInitSettings,
                      PASSPHRASE_ON_DEVICE, Capability, BackupType, RecoveryDeviceType)
-from ...gui.qt.wizard.wallet import WCScriptAndDerivation
-from ...gui.qt.wizard.wizard import WizardComponent
-from ...logging import Logger
+
 
 PASSPHRASE_HELP_SHORT =_(
     "Passphrases allow you to access new wallets, each "
@@ -261,15 +264,7 @@ class QtPlugin(QtPluginBase):
 
         wizard.exec_layout(vbox)
 
-        return TrezorInitSettings(
-            word_count=vbox.bg_numwords.checkedId(),
-            label=vbox.name.text(),
-            pin_enabled=vbox.cb_pin.isChecked(),
-            passphrase_enabled=vbox.cb_phrase.isChecked(),
-            recovery_type=vbox.bg_rectype.checkedId() if vbox.bg_rectype else None,
-            backup_type=vbox.bg_backuptype.checkedId(),
-            no_backup=vbox.cb_no_backup.isChecked() if vbox.cb_no_backup else False,
-        )
+        return vbox.get_settings()
 
 
 class InitSettingsLayout(QVBoxLayout):
@@ -443,6 +438,17 @@ class InitSettingsLayout(QVBoxLayout):
 
         self.addWidget(expert_widget)
 
+    def get_settings(self):
+        return TrezorInitSettings(
+            word_count=self.bg_numwords.checkedId(),
+            label=self.name.text(),
+            pin_enabled=self.cb_pin.isChecked(),
+            passphrase_enabled=self.cb_phrase.isChecked(),
+            recovery_type=self.bg_rectype.checkedId() if self.bg_rectype else None,
+            backup_type=self.bg_backuptype.checkedId(),
+            no_backup=self.cb_no_backup.isChecked() if self.cb_no_backup else False,
+        )
+
 
 class Plugin(TrezorPlugin, QtPlugin):
     icon_unpaired = "trezor_unpaired.png"
@@ -798,9 +804,13 @@ class WCTrezorXPub(WizardComponent, Logger):
         self.plugins = wizard.plugins
         self.plugin = self.plugins.get_plugin('trezor')
         self._busy = True
+
         self.xpub = None
+        self.root_fingerprint = None
+        self.label = None
+        self.soft_device_id = None
 
-        self.ok_l = WWLabel('Retrieved Hardware Information')
+        self.ok_l = WWLabel(_('Hardware keystore added to wallet'))
         self.ok_l.setAlignment(Qt.AlignCenter)
         self.layout().addWidget(self.ok_l)
 
@@ -810,7 +820,7 @@ class WCTrezorXPub(WizardComponent, Logger):
         client = self.plugins.device_manager.client_by_id(device_id, scan_now=False)
         client.handler = self.plugin.create_handler(self.wizard)
 
-        cosigner = self._current_cosigner(self.wizard_data)
+        cosigner = self.wizard.current_cosigner(self.wizard_data)
         xtype = cosigner['script_type']
         derivation = cosigner['derivation_path']
 
@@ -836,13 +846,19 @@ class WCTrezorXPub(WizardComponent, Logger):
 
     def validate(self):
         if self.xpub and not self.error:
-            self.valid = True
+            self.apply()
+            valid, error = self.wizard.check_multisig_constraints(self.wizard_data)
+            if not valid:
+                self.error = '\n'.join([
+                    _('Could not add hardware keystore to wallet'),
+                    error
+                ])
+            self.valid = valid
         else:
             self.valid = False
 
     def apply(self):
-        if self.valid:
-            cosigner_data = self._current_cosigner(self.wizard_data)
+        cosigner_data = self.wizard.current_cosigner(self.wizard_data)
         cosigner_data['hw_type'] = 'trezor'
         cosigner_data['master_key'] = self.xpub
         cosigner_data['root_fingerprint'] = self.root_fingerprint
@@ -863,16 +879,14 @@ class WCTrezorInitMethod(WizardComponent, Logger):
             (TIM_NEW, _("Let the device generate a completely new seed randomly")),
             (TIM_RECOVER, _("Recover from a seed you have previously written down")),
         ]
-        self.c_values = [x[0] for x in choices]
-        c_titles = [x[1] for x in choices]
-        self.clayout = ChoicesLayout(message, c_titles)
-        self.layout().addLayout(self.clayout.layout())
+        self.choice_w = ChoiceWidget(message=message, choices=choices)
+        self.layout().addWidget(self.choice_w)
         self.layout().addStretch(1)
 
         self._valid = True
 
     def apply(self):
-        self.wizard_data['trezor_init'] = self.c_values[self.clayout.selected_index()]
+        self.wizard_data['trezor_init'] = self.choice_w.selected_item[0]
 
 
 class WCTrezorInitParams(WizardComponent):
@@ -891,17 +905,7 @@ class WCTrezorInitParams(WizardComponent):
         self.busy = False
 
     def apply(self):
-        vbox = self.settings_layout
-        trezor_settings = TrezorInitSettings(
-            word_count=vbox.bg_numwords.checkedId(),
-            label=vbox.name.text(),
-            pin_enabled=vbox.cb_pin.isChecked(),
-            passphrase_enabled=vbox.cb_phrase.isChecked(),
-            recovery_type=vbox.bg_rectype.checkedId() if vbox.bg_rectype else None,
-            backup_type=vbox.bg_backuptype.checkedId(),
-            no_backup=vbox.cb_no_backup.isChecked() if vbox.cb_no_backup else False,
-        )
-        self.wizard_data['trezor_settings'] = trezor_settings
+        self.wizard_data['trezor_settings'] = self.settings_layout.get_settings()
 
 
 class WCTrezorInit(WizardComponent, Logger):

electrum/plugins/trezor/trezor.py

@@ -538,6 +538,7 @@ class TrezorPlugin(HW_PluginBase):
             },
             'trezor_xpub': {
                 'next': lambda d: wizard.wallet_password_view(d) if wizard.last_cosigner(d) else 'multisig_cosigner_keystore',
+                'accept': wizard.maybe_master_pubkey,
                 'last': lambda d: wizard.is_single_password() and wizard.last_cosigner(d)
             },
             'trezor_not_initialized': {

electrum/wizard.py

@@ -1,8 +1,9 @@
 import copy
 import os
 
-from typing import List, NamedTuple, Any, Dict, Optional
+from typing import List, NamedTuple, Any, Dict, Optional, Tuple
 
+from electrum.i18n import _
 from electrum.keystore import hardware_keystore
 from electrum.logging import get_logger
 from electrum.plugin import run_hook
@@ -270,7 +271,7 @@ class NewWalletWizard(AbstractWizard):
         raise NotImplementedError()
 
     # returns (sub)dict of current cosigner (or root if first)
-    def _current_cosigner(self, wizard_data):
+    def current_cosigner(self, wizard_data):
         wdata = wizard_data
         if wizard_data['wallet_type'] == 'multisig' and 'multisig_current_cosigner' in wizard_data:
             cosigner = wizard_data['multisig_current_cosigner']
@@ -278,11 +279,11 @@ class NewWalletWizard(AbstractWizard):
         return wdata
 
     def needs_derivation_path(self, wizard_data):
-        wdata = self._current_cosigner(wizard_data)
+        wdata = self.current_cosigner(wizard_data)
         return 'seed_variant' in wdata and wdata['seed_variant'] in ['bip39', 'slip39']
 
     def wants_ext(self, wizard_data):
-        wdata = self._current_cosigner(wizard_data)
+        wdata = self.current_cosigner(wizard_data)
         return 'seed_variant' in wdata and wdata['seed_extend']
 
     def is_multisig(self, wizard_data):
@@ -343,8 +344,8 @@ class NewWalletWizard(AbstractWizard):
         }.get(t)
 
     def on_have_cosigner_seed(self, wizard_data):
-        current_cosigner_data = wizard_data['multisig_cosigner_data'][str(wizard_data['multisig_current_cosigner'])]
-        if self.needs_derivation_path(wizard_data) and 'derivation_path' not in current_cosigner_data:
+        current_cosigner = self.current_cosigner(wizard_data)
+        if self.needs_derivation_path(wizard_data) and 'derivation_path' not in current_cosigner:
             return 'multisig_cosigner_script_and_derivation'
         elif self.last_cosigner(wizard_data):
             return 'wallet_password'
@@ -421,6 +422,47 @@ class NewWalletWizard(AbstractWizard):
         else:
             raise Exception('no seed or master_key in data')
 
+    def is_current_cosigner_hardware(self, wizard_data):
+        cosigner_data = self.current_cosigner(wizard_data)
+        cosigner_is_hardware = cosigner_data == wizard_data and wizard_data['keystore_type'] == 'hardware'
+        if 'cosigner_keystore_type' in wizard_data and wizard_data['cosigner_keystore_type'] == 'hardware':
+            cosigner_is_hardware = True
+        return cosigner_is_hardware
+
+    def check_multisig_constraints(self, wizard_data: dict) -> Tuple[bool, str]:
+        if not self.is_multisig(wizard_data):
+            return True, ''
+
+        # current cosigner might be incomplete. In that case, return valid
+        cosigner_data = self.current_cosigner(wizard_data)
+        if self.needs_derivation_path(wizard_data):
+            if 'derivation_path' not in cosigner_data:
+                self.logger.debug('defer multisig check: missing derivation_path')
+                return True, ''
+        if self.wants_ext(wizard_data):
+            if 'seed_extra_words' not in cosigner_data:
+                self.logger.debug('defer multisig check: missing extra words')
+                return True, ''
+        if self.is_current_cosigner_hardware(wizard_data):
+            if 'master_key' not in cosigner_data:
+                self._logger.debug('defer multisig check: missing master_key')
+                return True, ''
+
+        user_info = ''
+
+        if self.has_duplicate_masterkeys(wizard_data):
+            self._logger.debug('Duplicate master keys!')
+            user_info = _('Duplicate master keys')
+            multisig_keys_valid = False
+        elif self.has_heterogeneous_masterkeys(wizard_data):
+            self._logger.debug('Heterogenous master keys!')
+            user_info = _('Heterogenous master keys')
+            multisig_keys_valid = False
+        else:
+            multisig_keys_valid = True
+
+        return multisig_keys_valid, user_info
+
     def validate_seed(self, seed, seed_variant, wallet_type):
         seed_type = ''
         seed_valid = False