qml wizard: enforce homogeneous master keys in multisig

- {xpub, Ypub, Zpub} categories cannot be mixed
- old mpk must not be used in multisig

electrum/bip32.py

@@ -289,7 +289,8 @@ class BIP32Node(NamedTuple):
         return hash_160(self.eckey.get_public_key_bytes(compressed=True))[0:4]
 
 
-def xpub_type(x):
+def xpub_type(x: str):
+    assert x is not None
     return BIP32Node.from_xkey(x).xtype
 
 

electrum/gui/qml/components/NewWalletWizard.qml

@@ -33,6 +33,10 @@ Wizard {
             walletwizard.path = wiz.path
             walletwizard.walletCreated()
         }
+        function onCreateError(error) {
+            var dialog = app.messageDialog.createObject(app, { text: error })
+            dialog.open()
+        }
     }
 }
 

electrum/gui/qml/components/wizard/WCHaveMasterKey.qml

@@ -42,10 +42,14 @@ WizardComponent {
 
         if (cosigner) {
             applyMasterKey(key)
-            if (wiz.hasDuplicateKeys(wizard_data)) {
+            if (wiz.hasDuplicateMasterKeys(wizard_data)) {
                 validationtext.text = qsTr('Error: duplicate master public key')
                 return false
             }
+            if (wiz.hasHeterogeneousMasterKeys(wizard_data)) {
+                validationtext.text = qsTr('Error: master public key types do not match')
+                return false
+            }
         }
 
         return valid = true

electrum/gui/qml/components/wizard/WCHaveSeed.qml

@@ -65,9 +65,12 @@ WizardComponent {
             return
         } else {
             apply()
-            if (wiz.hasDuplicateKeys(wizard_data)) {
+            if (wiz.hasDuplicateMasterKeys(wizard_data)) {
                 validationtext.text = qsTr('Error: duplicate master public key')
                 return
+            } else if (wiz.hasHeterogeneousMasterKeys(wizard_data)) {
+                validationtext.text = qsTr('Error: master public key types do not match')
+                return
             } else {
                 valid = true
             }

electrum/gui/qml/qebitcoin.py

@@ -113,22 +113,30 @@ class QEBitcoin(QObject):
             return False
 
         k = keystore.from_master_key(key)
-        if isinstance(k, keystore.Xpub):  # has xpub  # TODO are these checks useful?
+        if wallet_type == 'standard':
-            t1 = xpub_type(k.xpub)
+            if isinstance(k, keystore.Xpub):  # has bip32 xpub
-            if wallet_type == 'standard':
+                t1 = xpub_type(k.xpub)
-                if t1 not in ['standard', 'p2wpkh', 'p2wpkh-p2sh']:
+                if t1 not in ['standard', 'p2wpkh', 'p2wpkh-p2sh']:  # disallow Ypub/Zpub
-                    self.validationMessage = '%s: %s' % (_('Wrong key type'), t1)
-                    return False
-                return True
-            elif wallet_type == 'multisig':
-                if t1 not in ['standard', 'p2wsh', 'p2wsh-p2sh']:
                     self.validationMessage = '%s: %s' % (_('Wrong key type'), t1)
                     return False
-                return True
+            elif isinstance(k, keystore.Old_KeyStore):
+                pass
             else:
-                self.validationMessage = '%s: %s' % (_('Unsupported wallet type'), wallet_type)
+                self._logger.error(f"unexpected keystore type: {type(keystore)}")
-                self.logger.error(f'Unsupported wallet type: {wallet_type}')
+                return False
+        elif wallet_type == 'multisig':
+            if not isinstance(k, keystore.Xpub):  # old mpk?
+                self.validationMessage = '%s: %s' % (_('Wrong key type'), "not bip32")
                 return False
+            t1 = xpub_type(k.xpub)
+            if t1 not in ['standard', 'p2wsh', 'p2wsh-p2sh']:  # disallow ypub/zpub
+                self.validationMessage = '%s: %s' % (_('Wrong key type'), t1)
+                return False
+        else:
+            self.validationMessage = '%s: %s' % (_('Unsupported wallet type'), wallet_type)
+            self._logger.error(f'Unsupported wallet type: {wallet_type}')
+            return False
+        # looks okay
         return True
 
     @pyqtSlot(str, result=bool)

electrum/gui/qml/qewizard.py

@@ -83,10 +83,16 @@ class QENewWalletWizard(NewWalletWizard, QEAbstractWizard):
         return self._daemon.singlePasswordEnabled
 
     @pyqtSlot('QJSValue', result=bool)
-    def hasDuplicateKeys(self, js_data):
+    def hasDuplicateMasterKeys(self, js_data):
-        self._logger.info('Checking for duplicate keys')
+        self._logger.info('Checking for duplicate masterkeys')
         data = js_data.toVariant()
-        return self.has_duplicate_keys(data)
+        return self.has_duplicate_masterkeys(data)
+
+    @pyqtSlot('QJSValue', result=bool)
+    def hasHeterogeneousMasterKeys(self, js_data):
+        self._logger.info('Checking for heterogeneous masterkeys')
+        data = js_data.toVariant()
+        return self.has_heterogeneous_masterkeys(data)
 
     @pyqtSlot('QJSValue', bool, str)
     def createStorage(self, js_data, single_password_enabled, single_password):

electrum/wizard.py

@@ -305,18 +305,38 @@ class NewWalletWizard(AbstractWizard):
 
         return True
 
-    def has_duplicate_keys(self, wizard_data):
+    def has_duplicate_masterkeys(self, wizard_data) -> bool:
+        """Multisig wallets need distinct master keys. If True, need to prevent wallet-creation."""
         xpubs = []
         xpubs.append(self.keystore_from_data(wizard_data).get_master_public_key())
         for cosigner in wizard_data['multisig_cosigner_data']:
             data = wizard_data['multisig_cosigner_data'][cosigner]
             xpubs.append(self.keystore_from_data(data).get_master_public_key())
-
+        assert xpubs
-        while len(xpubs):
+        return len(xpubs) != len(set(xpubs))
-            xpub = xpubs.pop()
+
-            if xpub in xpubs:
+    def has_heterogeneous_masterkeys(self, wizard_data) -> bool:
+        """Multisig wallets need homogeneous master keys.
+        All master keys need to be bip32, and e.g. Ypub cannot be mixed with Zpub.
+        If True, need to prevent wallet-creation.
+        """
+        xpubs = []
+        xpubs.append(self.keystore_from_data(wizard_data).get_master_public_key())
+        for cosigner in wizard_data['multisig_cosigner_data']:
+            data = wizard_data['multisig_cosigner_data'][cosigner]
+            xpubs.append(self.keystore_from_data(data).get_master_public_key())
+        assert xpubs
+        try:
+            k_xpub_type = xpub_type(xpubs[0])
+        except Exception:
+            return True  # maybe old_mpk?
+        for xpub in xpubs:
+            try:
+                my_xpub_type = xpub_type(xpub)
+            except Exception:
+                return True  # maybe old_mpk?
+            if my_xpub_type != k_xpub_type:
                 return True
-
         return False
 
     def keystore_from_data(self, data):
@@ -422,10 +442,17 @@ class NewWalletWizard(AbstractWizard):
             db.put('x3/', data['x3/'])
             db.put('use_trustedcoin', True)
         elif data['wallet_type'] == 'multisig':
+            if not isinstance(k, keystore.Xpub):
+                raise Exception(f"unexpected keystore(main) type={type(k)} in multisig. not bip32.")
+            k_xpub_type = xpub_type(k.xpub)
             db.put('wallet_type', '%dof%d' % (data['multisig_signatures'],data['multisig_participants']))
             db.put('x1/', k.dump())
             for cosigner in data['multisig_cosigner_data']:
                 cosigner_keystore = self.keystore_from_data(data['multisig_cosigner_data'][cosigner])
+                if not isinstance(cosigner_keystore, keystore.Xpub):
+                    raise Exception(f"unexpected keystore(cosigner) type={type(cosigner_keystore)} in multisig. not bip32.")
+                if k_xpub_type != xpub_type(cosigner_keystore.xpub):
+                    raise Exception("multisig wallet needs to have homogeneous xpub types")
                 if data['encrypt'] and cosigner_keystore.may_have_password():
                     cosigner_keystore.update_password(None, data['password'])
                 db.put(f'x{cosigner}/', cosigner_keystore.dump())