From 3edbb4e560658c388311be2f89d7671a29aeb64b Mon Sep 17 00:00:00 2001 From: scgbckbone Date: Tue, 9 Jul 2024 16:42:38 +0200 Subject: [PATCH] opt-in allow multi(non-BIP-67 multisig) --- docs/limitations.md | 10 +- releases/Next-ChangeLog.md | 1 + shared/actions.py | 8 +- shared/decoders.py | 3 +- shared/descriptor.py | 91 ++++--- shared/multisig.py | 159 +++++++++--- shared/nfc.py | 3 +- shared/nvstore.py | 1 + shared/psbt.py | 2 +- shared/usb.py | 2 +- testing/test_multisig.py | 491 +++++++++++++++++++++++++++---------- 11 files changed, 558 insertions(+), 213 deletions(-) diff --git a/docs/limitations.md b/docs/limitations.md index 37fa2c30..c9c3cb5c 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -62,7 +62,7 @@ that to the user for approval. - during USB "show address" for multisig, we limit subkey paths to 16 levels deep (including master fingerprint) -- max of 15 co-signers due to 520 byte script limitation in consensus layer with classic P2SH +- max of 15 co-signers due to 520 byte script limitation in consensus layer with classic P2SH (same limit applies to segwit even though consensus allows up to 20 co-signers) - (mk3) we have space for up to 8 M-of-3 wallets, or a single M-of-15 wallet. YMMV - only a single multisig wallet can be involved in a PSBT; can't sign inputs from two different multisig wallets at the same time. @@ -72,8 +72,14 @@ - derivation path for each cosigner must be known and consistent with PSBT - XFP values (fingerprints) MUST be unique for each of the co-signers - multisig wallet `name` can only contain printable ASCII characters `range(32, 127)` -- we support only BIP-67 (sorted multisig) wallets. +### BIP-67 +- importing multisig from PSBT can ONLY create `sortedmulti(...)` multisig according to BIP-67, DO NOT use with `multi(...)` +- creating airgapped multisig using COLDCARD as coordinator always produces `sortedmulti(...)` multisig according to BIP-67 +- COLDCARD import/export [format](https://coldcard.com/docs/multisig/#configuration-text-file-for-multisig) only supports `sortedmulti(...)` multisig according to BIP-67. To import multisig wallet with `multi(...)` use descriptor import [format](https://github.com/bitcoin/bips/blob/master/bip-0383.mediawiki) +- encrypted COLDCARD backups that contains multisig wallets with `multi(...)` MUST only be restored on firmware versions with `multi(...)` support +- all imported `multi(...)` must differ in keys (same as `sortedmulti(...)`). If `wsh(multi(2,A,B))` is already registered, `wsh(multi(2,B,A))` will be rejected upon import as duplicate, even thought they are actually different script/wallet. +- just BIP67 difference is also treated as duplicate. If `wsh(multi(2,A,B)` is registered, `wsh(sortedmulti(2,A,B))` will be rejected as duplicate and vice-versa. # SIGHASH types diff --git a/releases/Next-ChangeLog.md b/releases/Next-ChangeLog.md index 53a7c096..ba165f14 100644 --- a/releases/Next-ChangeLog.md +++ b/releases/Next-ChangeLog.md @@ -4,6 +4,7 @@ This lists the new changes that have not yet been published in a normal release. # Shared Improvements - Both Mk4 and Q +- New Feature: Opt-in support for `multi(...)` - unsorted multisig. Enable here `Settings->Multisig Wallets->Legacy Multisig` - Enhancement: Allow JSON files in `NFC File Share` - Enhancement: latest [0.5.0](https://github.com/bitcoin-core/secp256k1/releases/tag/v0.5.0) libsecp256k1 - Enhancement: Signature grinding optimizations. diff --git a/shared/actions.py b/shared/actions.py index a07c9766..7840cbf6 100644 --- a/shared/actions.py +++ b/shared/actions.py @@ -1624,9 +1624,11 @@ async def file_picker(suffix=None, min_size=1, max_size=1000000, taster=None, # ignore subdirs continue - if suffix and not fn.lower().endswith(suffix): - # wrong suffix - continue + if suffix: + if not isinstance(suffix, list): + suffix = [suffix] + if not any([fn.lower().endswith(s) for s in suffix]): + continue if fn[0] == '.': continue diff --git a/shared/decoders.py b/shared/decoders.py index 112fd9a9..6903e37c 100644 --- a/shared/decoders.py +++ b/shared/decoders.py @@ -195,7 +195,8 @@ def decode_short_text(got): pass # multisig descriptor - if ("sortedmulti(" in got): + # multi( catches both multi( and sortedmulti( + if ("multi(" in got): return 'multi', (got,) if ("\n" in got) and ('pub' in got): diff --git a/shared/descriptor.py b/shared/descriptor.py index c76dbff1..d344b41c 100644 --- a/shared/descriptor.py +++ b/shared/descriptor.py @@ -140,7 +140,7 @@ class Descriptor: self.addr_fmt = addr_fmt @staticmethod - def checksum_check(desc_w_checksum: str): + def checksum_check(desc_w_checksum): try: desc, checksum = desc_w_checksum.split("#") except ValueError: @@ -151,7 +151,7 @@ class Descriptor: return desc, checksum @staticmethod - def parse_key_orig_info(key: str): + def parse_key_orig_info(key): # key origin info is required for our MultisigWallet close_index = key.find("]") if key[0] != "[" or close_index == -1: @@ -162,7 +162,7 @@ class Descriptor: return key_orig_info, key @staticmethod - def parse_key_derivation_info(key: str): + def parse_key_derivation_info(key): invalid_subderiv_msg = "Invalid subderivation path - only 0/* or <0;1>/* allowed" slash_split = key.split("/") assert len(slash_split) > 1, invalid_subderiv_msg @@ -198,19 +198,19 @@ class Descriptor: result.append(key_str.replace("'", "h")) return result - def _serialize(self, internal=False, int_ext=False) -> str: + def _serialize(self, internal=False, int_ext=False): """Serialize without checksum""" assert len(self.keys) == 1 # "Multiple keys for single signature script" desc_base = SINGLE_FMT_TO_SCRIPT[self.addr_fmt] inner = self.serialize_keys(internal=internal, int_ext=int_ext)[0] return desc_base % (inner) - def serialize(self, internal=False, int_ext=False) -> str: + def serialize(self, internal=False, int_ext=False): """Serialize with checksum""" return append_checksum(self._serialize(internal=internal, int_ext=int_ext)) @classmethod - def parse(cls, desc_w_checksum: str) -> "Descriptor": + def parse(cls, desc_w_checksum): # remove garbage desc_w_checksum = parse_desc_str(desc_w_checksum) # check correct checksum @@ -234,7 +234,7 @@ class Descriptor: tmp_desc = tmp_desc.rstrip("))") else: - raise ValueError("Unsupported descriptor. Supported: pkh(, wpkh(, sh(wpkh(.") + raise ValueError("Unsupported descriptor. Supported: pkh(), wpkh(), sh(wpkh()).") koi, key = cls.parse_key_orig_info(tmp_desc) if key[0:4] not in ["tpub", "xpub"]: @@ -248,16 +248,23 @@ class Descriptor: @classmethod def is_descriptor(cls, desc_str): - """Method to guess whether this can be a descriptor""" + # Quick method to guess whether this is a descriptor try: temp = parse_desc_str(desc_str) - desc, checksum = temp.split("#") - assert desc[-1] == ")" - - return True except: return False + for prefix in ("pk(", "pkh(", "wpkh(", "tr(", "addr(", "raw(", "rawtr(", "combo(", + "sh(", "wsh(", "multi(", "sortedmulti(", "multi_a(", "sortedmulti_a("): + if temp.startswith(prefix): + return True + if prefix in temp: + # weaker case - needed for JSON wrapped imports + # if descriptor is invalid or unsuitable for our purpose + # we fail later (in parsing) + return True + return False + def bitcoin_core_serialize(self, external_label=None): # this will become legacy one day # instead use <0;1> descriptor format @@ -286,39 +293,44 @@ class MultisigDescriptor(Descriptor): "N", "keys", "addr_fmt", + "is_sorted" # whether to use sortedmulti() or multi() ) - def __init__(self, M, N, keys, addr_fmt): + def __init__(self, M, N, keys, addr_fmt, is_sorted=True): self.M = M self.N = N + self.is_sorted = is_sorted super().__init__(keys, addr_fmt) @classmethod - def parse(cls, desc_w_checksum: str) -> "MultisigDescriptor": + def parse(cls, desc_w_checksum): # remove garbage desc_w_checksum = parse_desc_str(desc_w_checksum) # check correct checksum desc, checksum = cls.checksum_check(desc_w_checksum) - # legacy - if desc.startswith("sh(sortedmulti("): - addr_fmt = AF_P2SH - tmp_desc = desc.replace("sh(sortedmulti(", "") - tmp_desc = tmp_desc.rstrip("))") - - # native segwit - elif desc.startswith("wsh(sortedmulti("): - addr_fmt = AF_P2WSH - tmp_desc = desc.replace("wsh(sortedmulti(", "") - tmp_desc = tmp_desc.rstrip("))") + is_sorted = "sortedmulti(" in desc + rplc = "sortedmulti(" if is_sorted else "multi(" # wrapped segwit - elif desc.startswith("sh(wsh(sortedmulti("): + if desc.startswith("sh(wsh("+rplc): addr_fmt = AF_P2WSH_P2SH - tmp_desc = desc.replace("sh(wsh(sortedmulti(", "") + tmp_desc = desc.replace("sh(wsh("+rplc, "") tmp_desc = tmp_desc.rstrip(")))") + # native segwit + elif desc.startswith("wsh("+rplc): + addr_fmt = AF_P2WSH + tmp_desc = desc.replace("wsh("+rplc, "") + tmp_desc = tmp_desc.rstrip("))") + + # legacy + elif desc.startswith("sh("+rplc): + addr_fmt = AF_P2SH + tmp_desc = desc.replace("sh("+rplc, "") + tmp_desc = tmp_desc.rstrip("))") + else: - raise ValueError("Unsupported descriptor. Supported: sh(, sh(wsh(, wsh(. All have to be sortedmulti.") + raise ValueError("Unsupported descriptor. Supported: sh(), sh(wsh()), wsh().") splitted = tmp_desc.split(",") M, keys = int(splitted[0]), splitted[1:] @@ -337,34 +349,41 @@ class MultisigDescriptor(Descriptor): origin_deriv = "m" + koi[8:] res_keys.append((xfp, origin_deriv, xpub)) - return cls(M=M, N=N, keys=res_keys, addr_fmt=addr_fmt) + return cls(M=M, N=N, keys=res_keys, addr_fmt=addr_fmt, is_sorted=is_sorted) - def _serialize(self, internal=False, int_ext=False) -> str: + def _serialize(self, internal=False, int_ext=False): """Serialize without checksum""" desc_base = MULTI_FMT_TO_SCRIPT[self.addr_fmt] - desc_base = desc_base % ("sortedmulti(%s)") + _type = "sortedmulti" if self.is_sorted else "multi" + _type += "(%s)" + desc_base = desc_base % _type assert len(self.keys) == self.N inner = str(self.M) + "," + ",".join( self.serialize_keys(internal=internal, int_ext=int_ext)) return desc_base % (inner) - def pretty_serialize(self) -> str: + def pretty_serialize(self): """Serialize in pretty and human-readable format""" + _type = "sortedmulti" if self.is_sorted else "multi" res = "# Coldcard descriptor export\n" - res += "# order of keys in the descriptor does not matter, will be sorted before creating script (BIP-67)\n" + if self.is_sorted: + res += "# order of keys in the descriptor does not matter, will be sorted before creating script (BIP-67)\n" + else: + res += ("# !!! DANGER: order of keys in descriptor MUST be preserved. " + "Correct order of keys is required to compose valid redeem/witness script.\n") if self.addr_fmt == AF_P2SH: res += "# bare multisig - p2sh\n" - res += "sh(sortedmulti(\n%s\n))" + res += "sh("+_type+"(\n%s\n))" # native segwit elif self.addr_fmt == AF_P2WSH: res += "# native segwit - p2wsh\n" - res += "wsh(sortedmulti(\n%s\n))" + res += "wsh("+_type+"(\n%s\n))" # wrapped segwit elif self.addr_fmt == AF_P2WSH_P2SH: res += "# wrapped segwit - p2sh-p2wsh\n" - res += "sh(wsh(sortedmulti(\n%s\n)))" + res += "sh(wsh(" + _type + "(\n%s\n)))" else: raise ValueError("Malformed descriptor") diff --git a/shared/multisig.py b/shared/multisig.py index 5d9fa067..f802f485 100644 --- a/shared/multisig.py +++ b/shared/multisig.py @@ -10,7 +10,7 @@ from ux import import_export_prompt, ux_enter_bip32_index, show_qr_code from files import CardSlot, CardMissingError, needs_microsd from descriptor import MultisigDescriptor, multisig_descriptor_template from public_constants import AF_P2SH, AF_P2WSH_P2SH, AF_P2WSH, AFC_SCRIPT, MAX_SIGNERS -from menu import MenuSystem, MenuItem, ShortcutItem +from menu import MenuSystem, MenuItem, NonDefaultMenuItem from opcodes import OP_CHECKMULTISIG from exceptions import FatalPSBTIssue from glob import settings @@ -84,9 +84,9 @@ def disassemble_multisig(redeem_script): return M, N, pubkeys -def make_redeem_script(M, nodes, subkey_idx): +def make_redeem_script(M, nodes, subkey_idx, bip67=True): # take a list of BIP-32 nodes, and derive Nth subkey (subkey_idx) and make - # a standard M-of-N redeem script for that. Always applies BIP-67 sorting. + # a standard M-of-N redeem script for that. Applies BIP-67 sorting by default. N = len(nodes) assert 1 <= M <= N <= MAX_SIGNERS @@ -98,7 +98,8 @@ def make_redeem_script(M, nodes, subkey_idx): pubkeys.append(b'\x21' + copy.pubkey()) del copy - pubkeys.sort() + if bip67: + pubkeys.sort() # serialize redeem script pubkeys.insert(0, bytes([80 + M])) @@ -127,7 +128,7 @@ class MultisigWallet(WalletABC): # optional: user can short-circuit many checks (system wide, one power-cycle only) disable_checks = False - def __init__(self, name, m_of_n, xpubs, addr_fmt=AF_P2SH, chain_type='BTC'): + def __init__(self, name, m_of_n, xpubs, addr_fmt=AF_P2SH, chain_type='BTC', bip67=True): self.storage_idx = -1 self.name = name @@ -137,6 +138,7 @@ class MultisigWallet(WalletABC): assert len(xpubs[0]) == 3 self.xpubs = xpubs # list of (xfp(int), deriv, xpub(str)) self.addr_fmt = addr_fmt # address format for wallet + self.bip67 = bip67 # calc useful cache value: numeric xfp+subpath, with lookup self.xfp_paths = {} @@ -197,11 +199,22 @@ class MultisigWallet(WalletABC): opts['d'] = pp xp = [(a, pp.index(deriv),c) for a,deriv,c in self.xpubs] - return (self.name, (self.M, self.N), xp, opts) + # make list already, will become one after json ser/deser + res = [self.name, (self.M, self.N), xp, opts] + if not self.bip67: + # wallets that do not follow BIP-67 are backwards incompatible + res.append(0) + + return res @classmethod def deserialize(cls, vals, idx=-1): # take json object, make instance. + bip67 = 1 # default enabled, requires 5-element serialization to disable + if len(vals) == 5: + bip67 = vals[-1] + vals = vals[:-1] + name, m_of_n, xpubs, opts = vals if len(xpubs[0]) == 2: @@ -221,9 +234,8 @@ class MultisigWallet(WalletABC): xpubs = [(a, derivs[b], c) for a,b,c in xpubs] rv = cls(name, m_of_n, xpubs, addr_fmt=opts.get('ft', AF_P2SH), - chain_type=opts.get('ch', 'BTC')) + chain_type=opts.get('ch', 'BTC'), bip67=bool(bip67)) rv.storage_idx = idx - return rv @classmethod @@ -394,7 +406,17 @@ class MultisigWallet(WalletABC): if c: # All details are same: M/N, paths, addr fmt if sorted(self.xpubs) != sorted(c.xpubs): + # this also applies to non-BIP-67 type multisig wallets + # multi(2,A,B) is treated as duplicate of multi(2,B,A) + # consensus-wise they are different script/wallet but CC + # don't allow to import one if other already imported return None, ['xpubs'], 0 + elif self.bip67 != c.bip67: + # treat same keys inside different desc multi/sortedmulti as duplicates + # sortedmulti(2,A,B) is considered same as multi(2,A,B) or multi(2,B,A) + # do not allow to import multi if sortedmulti with the same set of keys + # already imported and vice-versa + return None, ["BIP-67 clash"], 1 elif self.name == c.name: return None, [], 1 else: @@ -408,7 +430,6 @@ class MultisigWallet(WalletABC): # See if the xpubs are changing, which is risky... other differences like # name are okay. diffs = set() - name_diff = None for c in similar: if c.M != self.M: diffs.add('M differs') @@ -472,7 +493,7 @@ class MultisigWallet(WalletABC): if idx > MAX_BIP32_IDX: break # make the redeem script, convert into address - script = make_redeem_script(self.M, nodes, idx) + script = make_redeem_script(self.M, nodes, idx, self.bip67) addr = ch.p2sh_address(self.addr_fmt, script) yield idx, addr, [p.format(idx=idx) for p in paths], script @@ -526,6 +547,9 @@ class MultisigWallet(WalletABC): here = None too_shallow = False for xp_idx, path in check_these: + if not self.bip67: + assert xp_idx == pk_order, "script key order" + # matched fingerprint, try to make pubkey that needs to match xpub = self.xpubs[xp_idx][-1] @@ -576,7 +600,7 @@ class MultisigWallet(WalletABC): msg += ', too shallow' raise AssertionError(msg) - if pk_order: + if self.bip67 and pk_order: # verify sorted order assert bytes(pubkey) > bytes(pubkeys[pk_order-1]), 'BIP-67 violation' @@ -678,13 +702,14 @@ class MultisigWallet(WalletABC): is_mine = cls.check_xpub(xfp, xpub, deriv, chains.current_chain().ctype, my_xfp, xpubs) if is_mine: has_mine += 1 - return None, desc.addr_fmt, xpubs, has_mine, desc.M, desc.N + return None, desc.addr_fmt, xpubs, has_mine, desc.M, desc.N, desc.is_sorted def to_descriptor(self): return MultisigDescriptor( M=self.M, N=self.N, keys=self.xpubs, addr_fmt=self.addr_fmt, + is_sorted=self.bip67, ) @classmethod @@ -707,12 +732,14 @@ class MultisigWallet(WalletABC): # - xpub: any bip32 serialization we understand, but be consistent # expect_chain = chains.current_chain().ctype - if "sortedmulti(" in config or MultisigDescriptor.is_descriptor(config): - # assume descriptor, classic config should not contain sortedmulti( and check for checksum separator - # ignore name - _, addr_fmt, xpubs, has_mine, M, N = cls.from_descriptor(config) + if MultisigDescriptor.is_descriptor(config): + _, addr_fmt, xpubs, has_mine, M, N, bip67 = cls.from_descriptor(config) + if not bip67 and not settings.get("legacy_ms", 0): + # BIP-67 disabled, but legacy_ms not allowed - raise + raise AssertionError('Legacy multisig "multi(...)" not allowed') else: # oldschool + bip67 = True lines = [line for line in config.split('\n') if line] # remove empty lines parsed_name, addr_fmt, xpubs, has_mine, M, N = cls.from_simple_text(lines) if parsed_name: @@ -745,7 +772,8 @@ class MultisigWallet(WalletABC): assert has_mine == 1, 'my key included more than once' # done. have all the parts - return cls(name, (M, N), xpubs, addr_fmt=addr_fmt, chain_type=expect_chain) + return cls(name, (M, N), xpubs, addr_fmt=addr_fmt, + chain_type=expect_chain, bip67=bip67) @classmethod def check_xpub(cls, xfp, xpub, deriv, expect_chain, my_xfp, xpubs): @@ -1022,9 +1050,11 @@ class MultisigWallet(WalletABC): assert has_mine == 1 # 'my key not included' name = 'PSBT-%d-of-%d' % (M, N) + # this will always create sortedmulti multisig (BIP-67) + # because BIP-174 came years after wide spread acceptance of BIP-67 policy ms = cls(name, (M, N), xpubs, chain_type=expect_chain, addr_fmt=addr_fmt or AF_P2SH) - # may just keep just in-memory version, no approval required, if we are + # may just keep in-memory version, no approval required, if we are # trusting PSBT's today, otherwise caller will need to handle UX w.r.t new wallet return ms, (trust_mode != TRUST_PSBT) @@ -1097,20 +1127,29 @@ class MultisigWallet(WalletABC): is_dup = False if name_change: story = 'Update NAME only of existing multisig wallet?' + elif num_dups and isinstance(diff_items, list): + # failures only + story = "Duplicate wallet." + if diff_items: + story += diff_items[0] + else: + story += ' All details are the same as existing!' + is_dup = True elif diff_items: # Concern here is overwrite when similar, but we don't overwrite anymore, so # more of a warning about funny business. story = '''\ WARNING: This new wallet is similar to an existing wallet, but will NOT replace it. Consider deleting previous wallet first. Differences: \ ''' + ', '.join(diff_items) - elif num_dups: - story = 'Duplicate wallet. All details are the same as existing!' - is_dup = True else: story = 'Create new multisig wallet?' derivs, dsum = self.get_deriv_paths() + if not self.bip67 and not is_dup: + # do not need to warn if duplicate, won;t be allowed to import anyways + story += "\nWARNING: BIP-67 disabled! Unsorted multisig - order of keys in descriptor/backup is crucial" + story += '''\n Wallet Name: {name} @@ -1125,8 +1164,8 @@ Addresses: Derivation: {dsum} -Press (1) to see extended public keys, '''.format(M=M, N=N, name=self.name, exp=exp, dsum=dsum, - at=self.render_addr_fmt(self.addr_fmt)) +Press (1) to see extended public keys, '''.format(M=M, N=N, name=self.name, exp=exp, dsum=dsum, + at=self.render_addr_fmt(self.addr_fmt)) story += 'OK to approve, X to cancel.' if not is_dup else 'X to cancel' @@ -1155,14 +1194,17 @@ Press (1) to see extended public keys, '''.format(M=M, N=N, name=self.name, exp= msg = uio.StringIO() if verbose: - msg.write(''' -Policy: {M} of {N} -Blockchain: {ctype} -Addresses: - {at}\n\n'''.format(M=self.M, N=self.N, ctype=self.chain_type, - at=self.render_addr_fmt(self.addr_fmt))) + if not self.bip67: + msg.write("WARNING: BIP-67 disabled! Unsorted multisig - order of keys in descriptor/backup is crucial.\n\n") - # concern: the order of keys here is non-deterministic + vmsg = ('Policy: {M} of {N}\n' + 'Blockchain: {ctype}\n' + 'Addresses: {at}\n\n') + vmsg = vmsg.format(M=self.M, N=self.N, ctype=self.chain_type, + at=self.render_addr_fmt(self.addr_fmt)) + msg.write(vmsg) + + # order of keys in self.xpubs is same as order of keys in CC import format or descriptor for idx, (xfp, deriv, xpub) in enumerate(self.xpubs): if idx: msg.write('\n---===---\n\n') @@ -1184,7 +1226,7 @@ async def no_ms_yet(*a): await ux_show_story("You don't have any multisig wallets yet.") def disable_checks_chooser(): - ch = [ 'Normal', 'Skip Checks'] + ch = ['Normal', 'Skip Checks'] def xset(idx, text): MultisigWallet.disable_checks = bool(idx) @@ -1216,7 +1258,7 @@ Press (4) to confirm entering this DANGEROUS mode. def psbt_xpubs_policy_chooser(): # Chooser for trust policy - ch = [ 'Verify Only', 'Offer Import', 'Trust PSBT'] + ch = ['Verify Only', 'Offer Import', 'Trust PSBT'] def xset(idx, text): settings.set('pms', idx) @@ -1250,6 +1292,47 @@ exists, otherwise 'Verify'.''') if ch == 'x': return start_chooser(psbt_xpubs_policy_chooser) +def legacy_ms_chooser(): + ch = ['Do Not Allow', 'Allow'] + + def xset(idx, text): + settings.set('legacy_ms', idx) + from actions import goto_top_menu + goto_top_menu() + + return settings.get('legacy_ms', 0), ch, xset + +async def legacy_ms_menu(*a): + from menu import start_chooser + + if not settings.get("legacy_ms", None): + ch = await ux_show_story( + 'With this setting ON, it is allowed to import and operate' + ' "multi(...)" unsorted multisig wallets that do not follow BIP-67.' + ' It is of CRUCIAL importance for unsorted wallets, to backup multisig descriptor' + ' and preserve order of the keys in it.' + ' Many popular wallets like Sparrow and Electrum do NOT support "multi(...)".' + '\n\nUSE AT YOUR OWN RISK. Disabling BIP-67 is discouraged!' + '\n\nPress (4) to confirm allowing "multi(...)"', escape='4') + + if ch != '4': return + + else: + # legacy_ms enabled - assume he is going to disable + # check any multi(...) imported + ms = settings.get("multisig", []) + multi_names = [m[0] for m in ms if len(m) == 5] + if multi_names: + # do not allow to disable if any multi(...) imported + # list by name what needs to be removed + await ux_show_story( + "Remove already saved multi(...) wallets first.\n\n%s" + % multi_names + ) + return + + start_chooser(legacy_ms_chooser) + class MultisigMenu(MenuSystem): @classmethod @@ -1273,6 +1356,8 @@ class MultisigMenu(MenuSystem): rv.append(MenuItem('Create Airgapped', f=create_ms_step1)) rv.append(MenuItem('Trust PSBT?', f=trust_psbt_menu)) rv.append(MenuItem('Skip Checks?', f=disable_checks_menu)) + rv.append(NonDefaultMenuItem('Legacy Multisig', 'legacy_ms', + f=legacy_ms_menu)) return rv @@ -1303,10 +1388,14 @@ async def make_ms_wallet_menu(menu, label, item): MenuItem('"%s"' % ms.name, f=ms_wallet_detail, arg=ms), MenuItem('View Details', f=ms_wallet_detail, arg=ms), MenuItem('Delete', f=ms_wallet_delete, arg=ms), - MenuItem('Coldcard Export', f=ms_wallet_ckcc_export, arg=(ms, {})), - MenuItem('Descriptors', menu=make_ms_wallet_descriptor_menu, arg=ms), - MenuItem('Electrum Wallet', f=ms_wallet_electrum_export, arg=ms), ] + if ms.bip67: + rv += [ + MenuItem('Coldcard Export', f=ms_wallet_ckcc_export, arg=(ms, {})), + MenuItem('Electrum Wallet', f=ms_wallet_electrum_export, arg=ms), + ] + # only way to export non-BIP-67 ms wallet is descriptors (+core export) + rv.append(MenuItem('Descriptors', menu=make_ms_wallet_descriptor_menu, arg=ms)) return rv async def make_ms_wallet_descriptor_menu(menu, label, item): diff --git a/shared/nfc.py b/shared/nfc.py index 039a5519..8d3a40d8 100644 --- a/shared/nfc.py +++ b/shared/nfc.py @@ -670,7 +670,8 @@ class NFCHandler: for urn, msg, meta in ndef.record_parser(data): if len(msg) < 70: continue msg = bytes(msg).decode() # from memory view - if 'pub' in msg or 'sortedmulti(' in msg: + # multi( catches both multi( and sortedmulti( + if 'pub' in msg or "multi(" in msg: winner = msg break diff --git a/shared/nvstore.py b/shared/nvstore.py index 449e469b..ff90ae02 100644 --- a/shared/nvstore.py +++ b/shared/nvstore.py @@ -62,6 +62,7 @@ from utils import call_later_ms # b85max = (bool) allow max BIP-32 int value in BIP-85 derivations # ptxurl = (str) URL for PushTx feature, clear to disable feature # hmx = (bool) Force display of current XFP in home menu, even w/o tmp seed active +# legacy_ms = (bool) Allow unsorted multisig with BIP-67 disabled # Stored w/ key=00 for access before login # _skip_pin = hard code a PIN value (dangerous, only for debug) diff --git a/shared/psbt.py b/shared/psbt.py index 3c1971c9..6cc30b88 100644 --- a/shared/psbt.py +++ b/shared/psbt.py @@ -481,7 +481,7 @@ class psbtOutputProxy(psbtProxy): # redeem script must be exactly what we expect # - pubkeys will be reconstructed from derived paths here - # - BIP-45, BIP-67 rules applied + # - BIP-45, BIP-67 rules applied (BIP-67 optional from now - depending on imported descriptor) # - p2sh-p2wsh needs witness script here, not redeem script value # - if details provided in output section, must our match multisig wallet try: diff --git a/shared/usb.py b/shared/usb.py index 2c3de23b..7dc1ea80 100644 --- a/shared/usb.py +++ b/shared/usb.py @@ -463,7 +463,7 @@ class USBHandler: assert offset == len(args) return b'asci' + start_show_p2sh_address(M, N, addr_fmt, xfp_paths, - witdeem_script) + witdeem_script) if cmd == 'show': # simple cases, older code: text subpath diff --git a/testing/test_multisig.py b/testing/test_multisig.py index 180c1774..3b6f3156 100644 --- a/testing/test_multisig.py +++ b/testing/test_multisig.py @@ -9,7 +9,7 @@ import sys sys.path.append("../shared") from descriptor import MultisigDescriptor, append_checksum, MULTI_FMT_TO_SCRIPT, parse_desc_str -import time, pytest, os, random, json, shutil, pdb, io, base64, struct, bech32 +import time, pytest, os, random, json, shutil, pdb, io, base64, struct, bech32, itertools from psbt import BasicPSBT, BasicPSBTInput, BasicPSBTOutput from ckcc.protocol import CCProtocolPacker, MAX_TXN_LEN from pprint import pprint @@ -163,57 +163,20 @@ def offer_ms_import(cap_story, dev): return doit @pytest.fixture -def import_ms_wallet(dev, make_multisig, offer_ms_import, press_select, - is_q1, request, need_keypress): - - def doit(M, N, addr_fmt=None, name=None, unique=0, accept=False, common=None, - keys=None, do_import=True, derivs=None, descriptor=False, - int_ext_desc=False, dev_key=False, way=None): - keys = keys or make_multisig(M, N, unique=unique, dev_key=dev_key, - deriv=common or (derivs[0] if derivs else None)) - name = name or f'test-{M}-{N}' - - if not do_import: - return keys - - if descriptor: - if not derivs: - if not common: - common = "m/45h" - key_list = [(xfp, common, dd.hwif(as_private=False)) for xfp, m, dd in keys] +def import_multisig(request, is_q1, need_keypress, offer_ms_import): + def doit(fname=None, way="sd", data=None, name=None): + assert fname or data + if fname: + if way == "sd": + microsd_path = request.getfixturevalue("microsd_path") + fpath = microsd_path(fname) else: - assert len(derivs) == N - key_list = [(xfp, derivs[idx], dd.hwif(as_private=False)) for idx, (xfp, m, dd) in enumerate(keys)] - desc = MultisigDescriptor(M=M, N=N, keys=key_list, addr_fmt=addr_fmt) - if int_ext_desc: - desc_str = desc.serialize(int_ext=True) - else: - desc_str = desc.serialize() - config = "%s\n" % desc_str + virtdisk_path = request.getfixturevalue("virtdisk_path") + fpath = virtdisk_path(fname) + with open(fpath, 'r') as f: + config = f.read() else: - # render as a file for import - config = f"name: {name}\npolicy: {M} / {N}\n\n" - - if addr_fmt: - config += f'format: {addr_fmt.title()}\n' - - # not good enuf anymore, but maybe in some cases, just need one at top - if common: - config += f'derivation: {common}\n' - - if not derivs: - config += '\n'.join('%s: %s' % (xfp2str(xfp), dd.hwif(as_private=False)) - for xfp, m, dd in keys) - else: - # for cases where derivation of each leg is not same/simple - assert not common and len(derivs) == N - for idx, (xfp, m, dd) in enumerate(keys): - config += 'Derivation: %s\n%s: %s\n\n' % (derivs[idx], - xfp2str(xfp), dd.hwif(as_private=False)) - - #print(config) - open('debug/last-ms.txt', 'wt').write(config) - + config = data if way is None: # USB title, story = offer_ms_import(config) else: @@ -262,9 +225,10 @@ def import_ms_wallet(dev, make_multisig, offer_ms_import, press_select, else: path_f = request.getfixturevalue('virtdisk_path') - fname = name + ".txt" - with open(path_f(fname), "w") as f: - f.write(config) + if not fname: + fname = (name or "ms_wal.txt") + ".txt" + with open(path_f(fname), "w") as f: + f.write(config) pick_menu_item("Import from File") time.sleep(.1) @@ -279,8 +243,76 @@ def import_ms_wallet(dev, make_multisig, offer_ms_import, press_select, pick_menu_item(fname) - time.sleep(.2) + time.sleep(.1) title, story = cap_story() + return title, story + + return doit + +@pytest.fixture +def import_ms_wallet(dev, make_multisig, offer_ms_import, press_select, + is_q1, request, need_keypress, import_multisig, + settings_set): + + def doit(M, N, addr_fmt=None, name=None, unique=0, accept=False, common=None, + keys=None, do_import=True, derivs=None, descriptor=False, + int_ext_desc=False, dev_key=False, way=None, bip67=True, + force_legacy_ms=True): + # param: bip67 if false, only usable together with descriptor=True + if not bip67: + assert descriptor, "needs descriptor=True" + + if (not bip67) and force_legacy_ms: + settings_set("legacy_ms", 1) + + keys = keys or make_multisig(M, N, unique=unique, dev_key=dev_key, + deriv=common or (derivs[0] if derivs else None)) + name = name or f'test-{M}-{N}' + + if not do_import: + return keys + + if descriptor: + if not derivs: + if not common: + common = "m/45h" + key_list = [(xfp, common, dd.hwif(as_private=False)) for xfp, m, dd in keys] + else: + assert len(derivs) == N + key_list = [(xfp, derivs[idx], dd.hwif(as_private=False)) for idx, (xfp, m, dd) in enumerate(keys)] + desc = MultisigDescriptor(M=M, N=N, keys=key_list, addr_fmt=addr_fmt, is_sorted=bip67) + if int_ext_desc: + desc_str = desc.serialize(int_ext=True) + else: + desc_str = desc.serialize() + config = "%s\n" % desc_str + else: + # render as a file for import + config = f"name: {name}\npolicy: {M} / {N}\n\n" + + if addr_fmt: + if isinstance(addr_fmt, int): + addr_fmt = addr_fmt_names[addr_fmt] + config += f'format: {addr_fmt.title()}\n' + + # not good enuf anymore, but maybe in some cases, just need one at top + if common: + config += f'derivation: {common}\n' + + if not derivs: + config += '\n'.join('%s: %s' % (xfp2str(xfp), dd.hwif(as_private=False)) + for xfp, m, dd in keys) + else: + # for cases where derivation of each leg is not same/simple + assert not common and len(derivs) == N + for idx, (xfp, m, dd) in enumerate(keys): + config += 'Derivation: %s\n%s: %s\n\n' % (derivs[idx], + xfp2str(xfp), dd.hwif(as_private=False)) + + #print(config) + open('debug/last-ms.txt', 'wt').write(config) + + title, story = import_multisig(data=config, way=way) assert 'Create new multisig' in story \ or 'Update existing multisig wallet' in story \ @@ -364,9 +396,9 @@ def test_ms_import_variations(N, make_multisig, offer_ms_import, press_cancel, i press_cancel() assert f'Policy: {N} of {N}\n' in story -def make_redeem(M, keys, path_mapper=None, - violate_bip67=False, tweak_redeem=None, tweak_xfps=None, - finalizer_hack=None, tweak_pubkeys=None): +def make_redeem(M, keys, path_mapper=None, violate_script_key_order=False, + tweak_redeem=None, tweak_xfps=None, finalizer_hack=None, + tweak_pubkeys=None, bip67=True): # Construct a redeem script, and ordered list of xfp+path to match. N = len(keys) @@ -394,10 +426,11 @@ def make_redeem(M, keys, path_mapper=None, #print("path: %s => pubkey %s" % (path_to_str(path, skip=0), B2A(pk))) - data.sort(key=lambda i:i[0]) + if bip67: + data.sort(key=lambda i:i[0]) - if violate_bip67: - # move them out of order + if violate_script_key_order: + # move them out of order works for both multi and sortedmulti data[0], data[1] = data[1], data[0] @@ -430,12 +463,13 @@ def make_redeem(M, keys, path_mapper=None, return rv, [pk for pk,_,_ in data], xfp_paths -def make_ms_address(M, keys, idx=0, is_change=0, addr_fmt=AF_P2SH, testnet=1, **make_redeem_args): +def make_ms_address(M, keys, idx=0, is_change=0, addr_fmt=AF_P2SH, testnet=1, + bip67=True, **make_redeem_args): # Construct addr and script need to represent a p2sh address if 'path_mapper' not in make_redeem_args: make_redeem_args['path_mapper'] = lambda cosigner: [HARD(45), cosigner, is_change, idx] - script, pubkeys, xfp_paths = make_redeem(M, keys, **make_redeem_args) + script, pubkeys, xfp_paths = make_redeem(M, keys, bip67=bip67, **make_redeem_args) if addr_fmt == AF_P2WSH: # testnet=2 --> regtest @@ -474,9 +508,10 @@ def test_ms_show_addr(dev, cap_story, press_select, addr_vs_path, bitcoind_p2sh, scr, pubkeys, xfp_paths = make_redeem(M, keys, **make_redeem_args) assert len(scr) <= 520, "script too long for standard!" - got_addr = dev.send_recv(CCProtocolPacker.show_p2sh_address( - M, xfp_paths, scr, addr_fmt=addr_fmt), - timeout=None) + got_addr = dev.send_recv( + CCProtocolPacker.show_p2sh_address(M, xfp_paths, scr, addr_fmt=addr_fmt), + timeout=None + ) title, story = cap_story() @@ -501,7 +536,6 @@ def test_ms_show_addr(dev, cap_story, press_select, addr_vs_path, bitcoind_p2sh, assert B2A(scr) == core_scr assert core_addr == got_addr - return doit @@ -527,20 +561,54 @@ def test_import_ranges(m_of_n, use_regtest, addr_fmt, clear_ms, import_ms_wallet @pytest.mark.bitcoind @pytest.mark.ms_danger def test_violate_bip67(clear_ms, use_regtest, import_ms_wallet, - test_ms_show_addr, has_ms_checks): + test_ms_show_addr, has_ms_checks, + fake_ms_txn, try_sign): # detect when pubkeys are not in order in the redeem script + clear_ms() M, N = 1, 15 - keys = import_ms_wallet(M, N, accept=1) + keys = import_ms_wallet(M, N, accept=True) - try: - # test an address that should be in that wallet. - time.sleep(.1) - with pytest.raises(BaseException) as ee: - test_ms_show_addr(M, keys, violate_bip67=1) - assert 'BIP-67' in str(ee.value) - finally: - clear_ms() + # test an address that should be in that wallet. + time.sleep(.1) + with pytest.raises(BaseException) as ee: + test_ms_show_addr(M, keys, violate_script_key_order=True) + assert 'BIP-67' in str(ee.value) + + psbt = fake_ms_txn(1, 3, M, keys, + outstyles=ADDR_STYLES_MS, + change_outputs=[1], + violate_script_key_order=True) + + with open('debug/last.psbt', 'wb') as f: + f.write(psbt) + + with pytest.raises(Exception) as e: + try_sign(psbt) + assert 'BIP-67' in e.value.args[0] + + +@pytest.mark.parametrize("has_change", [True, False]) +def test_violate_import_order_multi(has_change, clear_ms, import_ms_wallet, + fake_ms_txn, try_sign, test_ms_show_addr): + clear_ms() + M, N = 3, 5 + keys = import_ms_wallet(M, N, accept=True, descriptor=True, bip67=False) + time.sleep(.1) + with pytest.raises(BaseException) as ee: + test_ms_show_addr(M, keys, violate_script_key_order=True) + assert "script key order" in str(ee.value) + + psbt = fake_ms_txn(4, 2, M, keys, outstyles=ADDR_STYLES_MS, + change_outputs=[1] if has_change else [], + bip67=False, violate_script_key_order=True) + + with open('debug/last.psbt', 'wb') as f: + f.write(psbt) + + with pytest.raises(Exception) as e: + try_sign(psbt) + assert "script key order" in e.value.args[0] @pytest.mark.bitcoind @@ -549,7 +617,7 @@ def test_bad_pubkey(has_ms_checks, use_regtest, clear_ms, import_ms_wallet, test_ms_show_addr, which_pubkey): # give incorrect pubkey inside redeem script M, N = 1, 15 - keys = import_ms_wallet(M, N, accept=1) + keys = import_ms_wallet(M, N, accept=True) try: # test an address that should be in that wallet. @@ -654,22 +722,35 @@ def test_bad_common_prefix(cpp, use_regtest, clear_ms, import_ms_wallet, assert 'bad derivation line' in str(ee) -def test_import_detail(clear_ms, import_ms_wallet, need_keypress, +@pytest.mark.parametrize("desc", ["multi", "sortedmulti"]) +def test_import_detail(desc, clear_ms, import_ms_wallet, need_keypress, cap_story, is_q1, press_cancel): # check all details are shown right M,N = 14, 15 - - keys = import_ms_wallet(M, N) + descriptor, bip67 = (True, False) if desc == "multi" else (False, True) + keys = import_ms_wallet(M, N, descriptor=descriptor, bip67=bip67) time.sleep(.2) - need_keypress('1') + title, story = cap_story() + assert f'{M} of {N}' in story + if desc == "multi": + assert "WARNING" in story + assert "BIP-67 disabled" in story + else: + assert "WARNING" not in story + assert "BIP-67 disabled" not in story + need_keypress('1') time.sleep(.1) title, story = cap_story() - #assert title == f'{M} of {N}' - assert title == f'test-{M}-{N}' + if desc == "sortedmulti": + assert title == f'test-{M}-{N}' + else: + # imported from descriptor - name will be just M N + assert title == f'{M}-of-{N}' + xpubs = [sk.hwif() for _,_,sk in keys] for xp in xpubs: assert xp in story @@ -970,7 +1051,7 @@ def test_import_dup_safe(N, clear_ms, make_multisig, offer_ms_import, menu = cap_menu() assert f'{M}/{N}: {name}' in menu # depending if NFC enabled or not, and if Q (has QR) - assert (len(menu) - num_wallets) in [5, 6, 7] + assert (len(menu) - num_wallets) in [6, 7, 8] title, story = offer_ms_import(make_named('xxx-orig')) assert 'Create new multisig wallet' in story @@ -1079,19 +1160,22 @@ def test_import_dup_xfp_fails(m_of_n, use_regtest, addr_fmt, clear_ms, #assert 'XFP' in str(ee) assert 'wrong pubkey' in str(ee) -@pytest.mark.parametrize('addr_fmt', [AF_P2SH, AF_P2WSH, AF_P2WSH_P2SH] ) -def test_ms_cli(dev, addr_fmt, clear_ms, import_ms_wallet, addr_vs_path, M=1, N=3): +@pytest.mark.parametrize('addr_fmt', [AF_P2SH, AF_P2WSH, AF_P2WSH_P2SH]) +@pytest.mark.parametrize('desc', ["multi", "sortedmulti"]) +def test_ms_cli(dev, addr_fmt, clear_ms, import_ms_wallet, addr_vs_path, desc): # exercise the p2sh command of ckcc:cli ... hard to do manually. - from subprocess import check_output + M, N = 2, 3 clear_ms() - keys = import_ms_wallet(M, N, name='cli-test', accept=1, - addr_fmt=addr_fmt_names[addr_fmt]) + bip67, descriptor = (False, True) if desc == "multi" else (True, False) + keys = import_ms_wallet(M, N, name='cli-test', accept=True, + addr_fmt=addr_fmt_names[addr_fmt], + descriptor=descriptor, bip67=bip67) pmapper = lambda i: [HARD(45), i, 0,3] - scr, pubkeys, xfp_paths = make_redeem(M, keys, pmapper) + scr, pubkeys, xfp_paths = make_redeem(M, keys, pmapper, bip67=bip67) def decode_path(p): return '/'.join(str(i) if i < 0x80000000 else "%d'"%(i& 0x7fffffff) for i in p) @@ -1120,10 +1204,10 @@ def test_ms_cli(dev, addr_fmt, clear_ms, import_ms_wallet, addr_vs_path, M=1, N= addr_vs_path(addr, addr_fmt=addr_fmt, script=scr) # test case for make_ms_address really. - expect_addr, _, scr2, _ = make_ms_address(M, keys, path_mapper=pmapper, addr_fmt=addr_fmt) + expect_addr, _, scr2, _ = make_ms_address(M, keys, path_mapper=pmapper, + addr_fmt=addr_fmt, bip67=bip67) assert expect_addr == addr assert scr2 == scr - # need to re-start our connection once ckcc has talked to simulator dev.start_encryption() @@ -1209,7 +1293,8 @@ def fake_ms_txn(pytestconfig): def doit(num_ins, num_outs, M, keys, fee=10000, outvals=None, segwit_in=False, outstyles=['p2pkh'], change_outputs=[], incl_xpubs=False, hack_psbt=None, - hack_change_out=False, input_amount=1E8, psbt_v2=None): + hack_change_out=False, input_amount=1E8, psbt_v2=None, bip67=True, + violate_script_key_order=False): psbt = BasicPSBT() if psbt_v2 is None: @@ -1245,7 +1330,8 @@ def fake_ms_txn(pytestconfig): # - each input is 1BTC # addr where the fake money will be stored. - addr, scriptPubKey, script, details = make_ms_address(M, keys, idx=i) + addr, scriptPubKey, script, details = make_ms_address(M, keys, idx=i, bip67=bip67, + violate_script_key_order=violate_script_key_order) # lots of supporting details needed for p2sh inputs if segwit_in: @@ -1295,10 +1381,12 @@ def fake_ms_txn(pytestconfig): make_redeem_args = dict() if hack_change_out: make_redeem_args = hack_change_out(i) + if violate_script_key_order: + make_redeem_args["violate_script_key_order"] = True addr, scriptPubKey, scr, details = \ make_ms_address(M, keys, idx=i, addr_fmt=unmap_addr_fmt[style], - **make_redeem_args) + bip67=bip67, **make_redeem_args) for pubkey, xfp_path in details: psbt.outputs[i].bip32_paths[pubkey] = b''.join(pack(' \n %s' % (idx, B2A(script))) @@ -2114,12 +2221,14 @@ def test_dup_ms_wallet_bug(goto_home, pick_menu_item, press_select, import_ms_wa @pytest.mark.parametrize('addr_fmt', [ AF_P2SH, AF_P2WSH, AF_P2WSH_P2SH ]) @pytest.mark.parametrize('int_ext_desc', [True, False]) @pytest.mark.parametrize('way', ["sd", "vdisk", "nfc"]) +@pytest.mark.parametrize('desc', ["multi", "sortedmulti"]) def test_import_desciptor(M_N, addr_fmt, int_ext_desc, way, import_ms_wallet, goto_home, pick_menu_item, press_select, clear_ms, cap_story, microsd_path, virtdisk_path, - nfc_read_text, load_export, is_q1): + nfc_read_text, load_export, is_q1, desc): clear_ms() M, N = M_N - import_ms_wallet(M, N, addr_fmt=addr_fmt, accept=1, descriptor=True, int_ext_desc=int_ext_desc) + import_ms_wallet(M, N, addr_fmt=addr_fmt, accept=1, descriptor=True, + int_ext_desc=int_ext_desc, bip67=False if desc == "multi" else True) goto_home() pick_menu_item('Settings') @@ -2140,11 +2249,12 @@ def test_import_desciptor(M_N, addr_fmt, int_ext_desc, way, import_ms_wallet, go assert desc_import == normalized starts_with = MULTI_FMT_TO_SCRIPT[addr_fmt].split("%")[0] assert normalized.startswith(starts_with) - assert "sortedmulti(" in desc_export + assert f"{desc}(" in desc_export @pytest.mark.bitcoind @pytest.mark.parametrize("change", [True, False]) +@pytest.mark.parametrize('desc', ["multi", "sortedmulti"]) @pytest.mark.parametrize("start_idx", [2147483540, MAX_BIP32_IDX, 0]) @pytest.mark.parametrize('M_N', [(2, 2), (3, 5), (15, 15)]) @pytest.mark.parametrize('addr_fmt', [AF_P2WSH, AF_P2SH, AF_P2WSH_P2SH]) @@ -2152,13 +2262,18 @@ def test_import_desciptor(M_N, addr_fmt, int_ext_desc, way, import_ms_wallet, go def test_bitcoind_ms_address(change, M_N, addr_fmt, clear_ms, goto_home, need_keypress, pick_menu_item, cap_menu, cap_story, make_multisig, import_ms_wallet, microsd_path, bitcoind_d_wallet_w_sk, use_regtest, load_export, way, - is_q1, press_select, start_idx, settings_set, set_addr_exp_start_idx): + is_q1, press_select, start_idx, settings_set, set_addr_exp_start_idx, + desc): use_regtest() clear_ms() bitcoind = bitcoind_d_wallet_w_sk M, N = M_N # whether to import as descriptor or old school to CC descriptor = random.choice([True, False]) + bip67 = True + if desc == "multi": + bip67 = False + descriptor = True settings_set("aei", True if start_idx else False) @@ -2177,7 +2292,7 @@ def test_bitcoind_ms_address(change, M_N, addr_fmt, clear_ms, goto_home, need_ke clear_ms() import_ms_wallet(M, N, accept=1, keys=keys, name=wal_name, derivs=derivs, - addr_fmt=text_a_fmt, descriptor=descriptor) + addr_fmt=text_a_fmt, descriptor=descriptor, bip67=bip67) goto_home() pick_menu_item("Address Explorer") @@ -2225,7 +2340,7 @@ def test_bitcoind_ms_address(change, M_N, addr_fmt, clear_ms, goto_home, need_ke desc_export = core_desc_object[0]["desc"] if descriptor: - assert "sortedmulti(" in desc_export + assert f"({desc}(" in desc_export if way == "nfc": end_idx = start_idx + 9 @@ -2370,11 +2485,14 @@ def test_legacy_multisig_witness_utxo_in_psbt(bitcoind, use_regtest, clear_ms, m @pytest.mark.parametrize("desc_type", ["p2wsh_desc", "p2sh_p2wsh_desc", "p2sh_desc"]) @pytest.mark.parametrize("sighash", list(SIGHASH_MAP.keys())) @pytest.mark.parametrize("psbt_v2", [True, False]) +@pytest.mark.parametrize('desc', ["multi", "sortedmulti"]) def test_bitcoind_MofN_tutorial(m_n, desc_type, clear_ms, goto_home, need_keypress, pick_menu_item, sighash, cap_menu, cap_story, microsd_path, use_regtest, bitcoind, microsd_wipe, load_export, settings_set, psbt_v2, is_q1, - finalize_v2_v0_convert, press_select): + finalize_v2_v0_convert, press_select, desc): # 2of2 case here is described in docs with tutorial + if desc == "multi": + settings_set("legacy_ms", 1) M, N = m_n settings_set("sighshchk", 1) # disable checks @@ -2409,6 +2527,9 @@ def test_bitcoind_MofN_tutorial(m_n, desc_type, clear_ms, goto_home, need_keypre press_select() xpub_obj = load_export("sd", label="Multisig XPUB", is_json=True, sig_check=False) template = xpub_obj[desc_type] + if desc == "multi": + # if we export descriptor template - it is always correct a.k.a sortedmulti + template = template.replace("sortedmulti(", "multi(") # get keys from bitcoind signers bitcoind_signers_xpubs = [] for signer in bitcoind_signers: @@ -2631,7 +2752,7 @@ def test_bitcoind_MofN_tutorial(m_n, desc_type, clear_ms, goto_home, need_keypre ("Invalid subderivation path - only 0/* or <0;1>/* allowed", "wsh(sortedmulti(2,[0f056943/48'/1'/0'/2']tpubDF2rnouQaaYrXF4noGTv6rQYmx87cQ4GrUdhpvXkhtChwQPbdGTi8GA88NUaSrwZBwNsTkC9bFkkC8vDyGBVVAQTZ2AS6gs68RQXtXcCvkP/0/*,[c463f778/44'/0'/0']tpubDD8pw7eZ9bUzYUR1LK5wpkA69iy3BpuLxPzsE6FFNdtTnJDySduc1VJdFEhEJQDKjYktznKdJgHwaQDRfQDQJpceDxH22c1ZKUMjrarVs7M))#gs2fqgl6"), ("Invalid subderivation path - only 0/* or <0;1>/* allowed", "wsh(sortedmulti(2,[0f056943/48'/1'/0'/2']tpubDF2rnouQaaYrXF4noGTv6rQYmx87cQ4GrUdhpvXkhtChwQPbdGTi8GA88NUaSrwZBwNsTkC9bFkkC8vDyGBVVAQTZ2AS6gs68RQXtXcCvkP/0/*,[c463f778/44'/0'/0']tpubDD8pw7eZ9bUzYUR1LK5wpkA69iy3BpuLxPzsE6FFNdtTnJDySduc1VJdFEhEJQDKjYktznKdJgHwaQDRfQDQJpceDxH22c1ZKUMjrarVs7M/0))#s487stua"), ("Cannot use hardened sub derivation path", "wsh(sortedmulti(2,[0f056943/48'/1'/0'/2']tpubDF2rnouQaaYrXF4noGTv6rQYmx87cQ4GrUdhpvXkhtChwQPbdGTi8GA88NUaSrwZBwNsTkC9bFkkC8vDyGBVVAQTZ2AS6gs68RQXtXcCvkP/0/*,[c463f778/44'/0'/0']tpubDD8pw7eZ9bUzYUR1LK5wpkA69iy3BpuLxPzsE6FFNdtTnJDySduc1VJdFEhEJQDKjYktznKdJgHwaQDRfQDQJpceDxH22c1ZKUMjrarVs7M/0'/*))#3w6hpha3"), - ("Unsupported descriptor", "wsh(multi(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/0/*))#t2zpj2eu"), + # ("Unsupported descriptor", "wsh(multi(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/0/*))#t2zpj2eu"), ("Unsupported descriptor", "pkh([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)#ml40v0wf"), ("M must be <= N", "wsh(sortedmulti(3,[0f056943/48'/1'/0'/2']tpubDF2rnouQaaYrXF4noGTv6rQYmx87cQ4GrUdhpvXkhtChwQPbdGTi8GA88NUaSrwZBwNsTkC9bFkkC8vDyGBVVAQTZ2AS6gs68RQXtXcCvkP/0/*,[c463f778/44'/0'/0']tpubDD8pw7eZ9bUzYUR1LK5wpkA69iy3BpuLxPzsE6FFNdtTnJDySduc1VJdFEhEJQDKjYktznKdJgHwaQDRfQDQJpceDxH22c1ZKUMjrarVs7M/0/*))#uueddtsy"), ]) @@ -2689,7 +2810,6 @@ def test_ms_wallet_ordering(clear_ms, import_ms_wallet, try_sign_microsd, fake_m @pytest.mark.parametrize("descriptor", [True, False]) @pytest.mark.parametrize("m_n", [(2, 3), (3, 5), (5, 10)]) def test_ms_xpub_ordering(descriptor, m_n, clear_ms, make_multisig, import_ms_wallet, try_sign_microsd, fake_ms_txn): - import itertools clear_ms() M, N = m_n all_out_styles = list(unmap_addr_fmt.keys()) @@ -2716,11 +2836,12 @@ def test_ms_xpub_ordering(descriptor, m_n, clear_ms, make_multisig, import_ms_wa @pytest.mark.parametrize('cmn_pth_from_root', [True, False]) @pytest.mark.parametrize('way', ["sd", "vdisk", "nfc"]) @pytest.mark.parametrize('M_N', [(3, 15), (2, 2), (3, 5), (15, 15)]) +@pytest.mark.parametrize('desc', ["multi", "sortedmulti"]) @pytest.mark.parametrize('addr_fmt', [AF_P2WSH, AF_P2SH, AF_P2WSH_P2SH]) def test_multisig_descriptor_export(M_N, way, addr_fmt, cmn_pth_from_root, clear_ms, make_multisig, import_ms_wallet, goto_home, pick_menu_item, cap_menu, nfc_read_text, microsd_path, cap_story, need_keypress, - load_export): + load_export, desc): def choose_multisig_wallet(): goto_home() @@ -2742,7 +2863,8 @@ def test_multisig_descriptor_export(M_N, way, addr_fmt, cmn_pth_from_root, clear derivs = [deriv.format(idx=i) for i in range(N)] clear_ms() import_ms_wallet(M, N, accept=1, keys=keys, name=wal_name, derivs=None if cmn_pth_from_root else derivs, - addr_fmt=text_a_fmt, descriptor=False, common="m/45h" if cmn_pth_from_root else None) + addr_fmt=text_a_fmt, descriptor=True, common="m/45h" if cmn_pth_from_root else None, + bip67=False if desc == "multi" else True) # get bare descriptor choose_multisig_wallet() pick_menu_item("Descriptors") @@ -2793,6 +2915,8 @@ def test_multisig_descriptor_export(M_N, way, addr_fmt, cmn_pth_from_root, clear view_desc = story.strip().split("\n\n")[1] # assert that bare and pretty are the same after parse + assert f"({desc}(" in bare_desc + assert bare_desc == view_desc assert parse_desc_str(pretty_desc) == bare_desc for obj in core_desc_object: @@ -2965,6 +3089,7 @@ def test_bare_cc_ms_qr_import(N, make_multisig, scan_a_qr, clear_ms, goto_home, @pytest.mark.parametrize("psbtv2", [True, False]) +@pytest.mark.parametrize("desc", ["multi", "sortedmulti"]) @pytest.mark.parametrize("data", [ # (out_style, amount, is_change) [("p2wsh", 1000000, 0)] * 99, @@ -2973,10 +3098,14 @@ def test_bare_cc_ms_qr_import(N, make_multisig, scan_a_qr, clear_ms, goto_home, [("p2sh", 1000000, 1), ("p2wsh-p2sh", 50000000, 0), ("p2wsh", 800000, 1)] * 14, ]) def test_txout_explorer(psbtv2, data, clear_ms, import_ms_wallet, fake_ms_txn, - start_sign, txout_explorer): + start_sign, txout_explorer, desc): clear_ms() M, N = 2, 3 - keys = import_ms_wallet(2, 3, name='ms-test', accept=1) + descriptor, bip67 = False, True + if desc == "multi": + descriptor, bip67 = True, False + keys = import_ms_wallet(2, 3, name='ms-test', accept=True, + descriptor=descriptor, bip67=bip67) outstyles = [] outvals = [] @@ -2991,29 +3120,24 @@ def test_txout_explorer(psbtv2, data, clear_ms, import_ms_wallet, fake_ms_txn, inp_amount = sum(outvals) + 100000 # 100k sat fee psbt = fake_ms_txn(1, len(data), M, keys, outstyles=outstyles, outvals=outvals, change_outputs=change_outputs, - input_amount=inp_amount, psbt_v2=psbtv2) + input_amount=inp_amount, psbt_v2=psbtv2, bip67=bip67) start_sign(psbt) txout_explorer(data) - -@pytest.mark.parametrize("desc", [True, False]) -def test_import_duplicate_shuffled_keys(desc, clear_ms, make_multisig, import_ms_wallet, - microsd_path, pick_menu_item, cap_story, goto_home, - press_cancel): - # DO NOT allow to import wsh(sortedmulti(2, A,B)) and wsh(sortedmulti(2, B, A)) - # MUST BE treated as duplicates +def test_import_duplicate_shuffled_keys_legacy(clear_ms, make_multisig, import_ms_wallet, + cap_story, press_cancel): clear_ms() M, N = 2, 3 wname = "ms02" keys = make_multisig(M, N) import_ms_wallet(M, N, addr_fmt="p2wsh", name=wname, accept=True, keys=keys, - descriptor=desc) + descriptor=False) # shuffle keys[0], keys[1] = keys[1], keys[0] with pytest.raises(AssertionError): import_ms_wallet(M, N, addr_fmt="p2wsh", name=wname, accept=True, keys=keys, - descriptor=desc) + descriptor=False) time.sleep(.1) title, story = cap_story() @@ -3021,4 +3145,105 @@ def test_import_duplicate_shuffled_keys(desc, clear_ms, make_multisig, import_ms assert 'OK to approve' not in story press_cancel() +@pytest.mark.parametrize("order", list(itertools.product([True, False], repeat=2))) +def test_import_duplicate_shuffled_keys(clear_ms, make_multisig, import_ms_wallet, + cap_story, press_cancel, order): + # DO NOT allow to import both wsh(sortedmulti(2,A,B,C)) and wsh(sortedmulti(2,B,C,A)) + # DO NOT allow to import both wsh(multi(2,A,B,C)) and wsh(multi(2,B,C,A)) + # DO NOT allow to import both wsh(sortedmulti(2,A,B,C)) and wsh(multi(2,B,C,A)) + # MUST BE treated as duplicates + clear_ms() + M, N = 2, 3 + A, B = order # defines bip67 + wname = "ms02" + keys = make_multisig(M, N) + import_ms_wallet(M, N, addr_fmt="p2wsh", name=wname, accept=True, keys=keys, + descriptor=True, bip67=A) + # shuffle + keys[0], keys[1] = keys[1], keys[0] + + with pytest.raises(AssertionError): + import_ms_wallet(M, N, addr_fmt="p2wsh", name=wname, accept=True, keys=keys, + descriptor=True, bip67=B) + time.sleep(.1) + title, story = cap_story() + assert 'Duplicate wallet' in story + assert 'OK to approve' not in story + if A != B: + assert "BIP-67 clash" in story + + press_cancel() + + +@pytest.mark.parametrize("int_ext", [True, False]) +def test_multi_sortedmulti_duplicate(clear_ms, make_multisig, import_ms_wallet, + cap_story, press_cancel, int_ext, offer_ms_import): + clear_ms() + M, N = 3, 5 + wname = "ms001" + fstr = "m/48h/1h/0h/2h/{idx}" + derivs = [fstr.format(idx=i) for i in range(N)] + keys = make_multisig(M, N, deriv=fstr) + import_ms_wallet(M, N, addr_fmt="p2wsh", name=wname, accept=True, + keys=keys, int_ext_desc=True, derivs=derivs) + + # create identical but unsorted descriptor + obj_keys = [(keys[i][0], derivs[i], keys[i][2].hwif()) + for i in range(len(keys))] + d = MultisigDescriptor(M, N, obj_keys, addr_fmt=AF_P2WSH, is_sorted=False) + ser_desc = d.serialize(int_ext=int_ext) + + title, story = offer_ms_import(ser_desc) + assert 'Duplicate wallet' in story + assert 'OK to approve' not in story + assert "BIP-67 clash" in story + press_cancel() + + +def test_legacy_multisig_setting(settings_set, import_ms_wallet, goto_home, + pick_menu_item, cap_story, need_keypress, + settings_get, clear_ms, press_select): + clear_ms() + settings_set("legacy_ms", 0) # OFF by default + with pytest.raises(Exception) as e: + import_ms_wallet(2, 3, "p2wsh", descriptor=True, bip67=False, + accept=True, force_legacy_ms=False) + assert '"multi(...)" not allowed' in e.value.args[0] + + goto_home() + pick_menu_item("Settings") + pick_menu_item("Multisig Wallets") + pick_menu_item("Legacy Multisig") + time.sleep(.1) + title, story = cap_story() + assert '"multi(...)" unsorted multisig wallets that do not follow BIP-67.' in story + assert 'preserve order of the keys' in story + assert 'USE AT YOUR OWN RISK' in story + assert 'Press (4)' in story + need_keypress("4") + time.sleep(.1) + pick_menu_item("Allow") + time.sleep(.3) + assert settings_get("legacy_ms") == 1 + import_ms_wallet(2, 3, "p2wsh", descriptor=True, bip67=False, + accept=True, force_legacy_ms=False) + assert len(settings_get("multisig")) == 1 + pick_menu_item("Settings") + pick_menu_item("Multisig Wallets") + pick_menu_item("Legacy Multisig") + time.sleep(.1) + title, story = cap_story() + assert "Remove already saved multi(...) wallets first" in story + assert "2-of-3" in story # wallet that needs to be removed + press_select() + assert len(settings_get("multisig")) == 1 + clear_ms() + pick_menu_item("Legacy Multisig") + pick_menu_item("Do Not Allow") + time.sleep(.3) + with pytest.raises(Exception) as e: + import_ms_wallet(2, 3, "p2wsh", descriptor=True, bip67=False, + accept=True, force_legacy_ms=False) + assert '"multi(...)" not allowed' in e.value.args[0] + # EOF