From bb2471a60eb42781dd7f37a397c221f87dfe32f3 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:03:48 -0500 Subject: [PATCH 1/6] Verify that change outputs actually pay this seed An output counted as change when its policy matched the inputs' and its rebuilt scriptPubKey matched what the output committed to. Neither step established that the key involved was ours. The policy comparison included cosigners resolved from the coordinator's own global xpubs, so one misannotated fingerprint made an output stop matching and skip verification altogether, and multisig never checked our key against the committed script at all. Outputs now compare on script shape alone, and every candidate proves ownership: single sig by rebuilding from the claimed derivation path, multisig by finding this seed's key in the committed script. Where the psbt's account of an output contradicts what the output commits to, the parse refuses rather than quietly reclassifying. A claim set too malformed to answer that question, one populating both derivation path maps or claiming more keys than its script uses, is refused as well. change_data now carries the verified derivation path in place of the coordinator's claimed fingerprints and paths, so the views no longer re-derive trust from strings the parse has already settled. Taproot mismatches stay exempt. A script tree tweaks the internal key, so honest taproot change fails the rebuild too, and embit leaves PSBT_OUT_TAP_TREE unparsed, which is what would tell the two apart. --- src/seedsigner/models/psbt_parser.py | 426 +++++++++++--- src/seedsigner/views/psbt_views.py | 269 +++++---- tests/psbt_testing_util.py | 82 ++- tests/screenshot_generator/generator.py | 9 +- tests/test_flows_psbt.py | 116 +++- tests/test_psbt_parser.py | 720 ++++++++++++++++++++++-- 6 files changed, 1386 insertions(+), 236 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 044a9ef5..d4fd01d2 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -51,6 +51,58 @@ class PSBTInputOwnershipClaimError(PSBTVerificationError): pass +class PSBTSurplusDerivationPathsError(PSBTVerificationError): + """ + Raised for three similar cases: + * single sig: output has more than one derivation path entry. + * multisig: an output confirmed to pay this seed claims more derivation path + entries than its committed script has keys. + * taproot: output has more than one derivation path entry claiming to be its + internal key (i.e. claiming no leaf hashes). + + For single sig this is clearly a structural error and is not expected to be seen in + the real world but it's worth the sanity check. + + For multisig, this could be an attempt to deceive the user, but we do not try to + adjudicate that. + """ + pass + + +class PSBTMixedDerivationPathTypesError(PSBTVerificationError): + """ + An input or output declares derivation paths in both the ecdsa and the taproot key + maps (bip32_derivations and taproot_bip32_derivations). + + This is a correctness problem (not expected to be seen in the real world) or + potentially a weak form of deception, but we do not try to adjudicate that. + """ + pass + + +class PSBTOutputOwnershipContradictionError(PSBTVerificationError): + """ + The psbt's account of who an output pays contradicts which key(s) the output + actually commits to. + + For multisig, it's one of: + * The output claims one of our keys, but the script the psbt supplied for it does + not match what the output actually commits to (the output's scriptPubKey). + * The output claims that one of our keys is part of the multisig that owns the + output, but that key is not part of the scriptPubKey commitment. + * Our seed owns a key that is part of the scriptPubKey commitment, but the output + claims a different seed in our place, by fingerprint or by listed public key. + + Single sig supplies no script, so both contradictions come from the rebuild alone: the + output claims our key but commits to another key, or it commits to our key while + claiming a different fingerprint in our place. + + + We treat any of these deceptions as an attack. + """ + pass + + class PSBTSeedCannotSignError(PSBTVerificationError): """ The selected seed holds no key that could sign any input. @@ -186,12 +238,18 @@ class PSBTParser(): - multisig, cosigners unresolved: script type and m-of-n only. _get_policy doesn't propagate cosigner errors, so two such policies match without anything having tied them to the same keys. TODO: don't let a - policy with no cosigner information pass as a match. + policy with no cosigner information pass as a match between inputs. + Outputs deliberately compare shape alone; see _policy_shape_matches. - 5. _parse_outputs: works out which outputs come back to this seed. For - single-sig this proves the output script derives from the seed at the - claimed path. TODO: reject outputs at a path the user's wallet would never - scan. + 5. _parse_outputs: organizes the output data (amounts, destination_addresses, + etc.) and verifies the ownership of the outputs that come back to this seed + via: + - single-sig: Rebuild the output script from the seed and match it against + the committed scriptPubKey. + - multisig: Match the seed's verified key against the pubkeys in the script + the output commits to. + Every change_data entry after this point will carry a derivation path that + our seed provably owns. Optimization via child_key_derivation_cache: Parsing traverses a derivation path down to an individual address one level at a @@ -266,7 +324,55 @@ class PSBTParser(): if self.policy != inp_policy: raise RuntimeError("Mixed inputs in the transaction") + def _parse_outputs(self, child_key_derivation_cache: dict): + """ + Sorts each output into change coming back to this seed, an external spend, or + OP_RETURN data, and totals the amounts for each. Note that self-transfer/receive + outputs are also considered "change". + + Most of the work here is determining which, if any, outputs are verifiably being + paid to our seed. + """ + + """****************** How outputs are verified as change ************************ + Many outputs are obviously NOT change. An output is only considered possible + change if its policy matches the inputs' policy "shape" (script type, plus m-of-n + for multisig; see parse()); anything else is recorded as an external spend. + + The psbt will usually annotate which key(s) own a change output (see embit's + bip32_derivations and taproot_bip32_derivations), but this is just a claim + supplied by the coordinator. These annotations are not authoritative. But such + claims are significant; if our checks prove that the claim is false, we consider + the deception an attack. + + -- Proving the claim -- + The output's scriptPubKey determines where the value ACTUALLY goes. But the + scriptPubKey only contains a hash of the spending conditions (note: taproot uses a + tweaked key instead), so we can't simply inspect the scriptPubKey to determine if + the output is ours. + + We must build our own version of the scriptPubKey via: + * single sig: derive a key from our seed using the claimed derivation path. + * multisig: hash the claimed witness_script or redeem_script. + + That leaves us holding two independent answers about the same output: which key it + commits to (our rebuild, matched against the scriptPubKey), and which key the psbt + says it commits to (the claim). We evaluate the output on those two facts: + + | claims this seed | doesn't claim this seed + -------------------------+----------------------+------------------------- + commits to our key | confirmed: is change | contradiction + commits to another key | contradiction | presumed external spend + + If our two answers contradict each other, the psbt has been caught in a deception. + We raise an exception and reject the psbt. + + (note one exception: no taproot mismatch is rejected. A script tree tweaks our + internal key, so an honest taproot change output fails to match too, and we cannot + yet tell that apart from an output that claims our key but pays someone else. All + taproot mismatches pass as EXTERNAL spends and are never considered "change".) + ******************************************************************************""" self.spend_amount = 0 self.change_amount = 0 self.change_data = [] @@ -283,88 +389,192 @@ class PSBTParser(): out_policy = PSBTParser._get_policy(out, vout[i].script_pubkey, self.psbt.xpubs, child_key_derivation_cache) is_change = False - # if policy is the same - probably change - if out_policy == self.policy: - # double-check that it's change - # we already checked in get_cosigners and parse_multisig - # that pubkeys are generated from cosigners, - # and witness script is corresponding multisig - # so we only need to check that scriptpubkey is generated from - # witness script + # Is this output change? If this output's policy is superficially similar to + # the spending wallet's policy (e.g. they're both 2-of-3 p2wsh), then it's a + # candidate for being change. + if PSBTParser._policy_shape_matches(out_policy, self.policy): + # Begin the extensive work to fully verify whether this output is indeed + # change. - # empty script by default - sc = script.Script(b"") + # Each of these is a claim we build our proof from, then keep for the + # follow-up check its signature type needs: + # * Single sig: the derivation path the seed derives a key at. + # * Multisig: the witness or redeem script the coordinator supplied. + # Only one of these will be needed, depending on the output type. + singlesig_derivation_path = None + multisig_script = None + + # Compared against the output's real scriptPubKey below + rebuilt_script_pubkey = script.Script(b"") # multisig, we know witness script if self.policy["type"] == "p2wsh": - sc = script.p2wsh(out.witness_script) + multisig_script = out.witness_script + rebuilt_script_pubkey = script.p2wsh(multisig_script) elif self.policy["type"] == "p2sh-p2wsh": - sc = script.p2sh(script.p2wsh(out.witness_script)) - + multisig_script = out.witness_script + rebuilt_script_pubkey = script.p2sh(script.p2wsh(multisig_script)) + # Arbitrary p2sh; includes pre-segwit multisig (m/45') elif self.policy["type"] == "p2sh": - sc = script.p2sh(out.redeem_script) + multisig_script = out.redeem_script + rebuilt_script_pubkey = script.p2sh(multisig_script) # single-sig: p2pkh, p2sh-p2wpkh, and p2wpkh; taproot handled separately # below. elif "pkh" in self.policy["type"]: - my_pubkey = None + # Sanity check; a single sig output shouldn't have multiple derivation + # paths. + if len(out.bip32_derivations) > 1: + raise PSBTSurplusDerivationPathsError("Single-key output claims more than one derivation path") - # should be one or zero for single-key addresses - if len(out.bip32_derivations.values()) > 0: - der = list(out.bip32_derivations.values())[0].derivation - my_pubkey = PSBTParser._derive_with_cache(self.root, der, child_key_derivation_cache) - - if self.policy["type"] == "p2pkh" and my_pubkey is not None: - sc = script.p2pkh(my_pubkey) - - elif self.policy["type"] == "p2sh-p2wpkh" and my_pubkey is not None: - sc = script.p2sh(script.p2wpkh(my_pubkey)) - - elif self.policy["type"] == "p2wpkh" and my_pubkey is not None: - sc = script.p2wpkh(my_pubkey) + # Rebuild the scriptPubKey from the key at the claimed derivation path + if len(out.bip32_derivations.values()) == 1: + singlesig_derivation_path = list(out.bip32_derivations.values())[0].derivation + seed_public_key = PSBTParser._derive_with_cache(self.root, singlesig_derivation_path, child_key_derivation_cache).get_public_key() + rebuilt_script_pubkey = PSBTParser._build_singlesig_script(self.policy["type"], seed_public_key) + else: + # There's nothing for us to verify against so this output will be + # considered an external spend. + pass elif "p2tr" in self.policy["type"]: - my_pubkey = None - # should have one or zero derivations for single-key addresses - if len(out.taproot_bip32_derivations.values()) > 0: - # TODO: Support keys in taptree leaves - leaf_hashes, derivation = list(out.taproot_bip32_derivations.values())[0] - der = derivation.derivation - my_pubkey = PSBTParser._derive_with_cache(self.root, der, child_key_derivation_cache) - sc = script.p2tr(my_pubkey) + taproot_entries = list(out.taproot_bip32_derivations.values()) - if sc.data == vout[i].script_pubkey.data: + if len(taproot_entries) == 0: + # There's nothing for us to verify against so this output will be + # considered an external spend. + pass + else: + # A taproot output has exactly one internal key. So an output + # should not claim multiple derivation path entries for the + # internal key. However, taproot outputs can have additional + # entries for keys in script tree leaves. So we count just the + # internal key claims: + internal_key_claims = sum(1 for leaf_hashes, _ in taproot_entries if not leaf_hashes) + if internal_key_claims > 1: + raise PSBTSurplusDerivationPathsError("Taproot output claims more than one internal key") + + if len(taproot_entries) == 1 and internal_key_claims == 1: + leaf_hashes, derivation = taproot_entries[0] + singlesig_derivation_path = derivation.derivation + seed_public_key = PSBTParser._derive_with_cache(self.root, singlesig_derivation_path, child_key_derivation_cache).get_public_key() + rebuilt_script_pubkey = PSBTParser._build_singlesig_script(self.policy["type"], seed_public_key) + else: + # This output has at least one derivation path entry for a key + # in a script tree leaf. But since we don't yet parse the + # script tree, we can't reconstruct the output's correct + # scriptPubKey. So this output will fail to match its + # scriptPubKey below, at which point it will be considered an + # external spend. This is the best we can do when we cannot + # verify ownership. + # TODO: Support keys in script tree leaves + pass + + verified_derivation_path = self.verified_output_derivation_paths[i] + + if rebuilt_script_pubkey.data == vout[i].script_pubkey.data: + # The scriptPubKey we created using our own seed matched what this + # output is actually committing to. + + # Remember that "change" is ANY output coming back to our seed is_change = True + if singlesig_derivation_path is not None: + if verified_derivation_path is None: + # The output pays this seed but the psbt claimed a different + # fingerprint here. We treat this deception as an attack. + raise PSBTOutputOwnershipContradictionError(f"Output pays this seed at {bip32.path_to_str(singlesig_derivation_path)} but does not claim it there") + + if verified_derivation_path != singlesig_derivation_path: + # Shouldn't be able to reach here: the surplus check above + # allows only one entry, and the ownership scan refuses a + # scope populating both derivation path maps, so the scan can + # only have verified this same path. + raise RuntimeError(f"Output {i} verified at a path it does not pay") + + elif multisig_script is not None: + if verified_derivation_path is None: + # No entry claimed this seed's fingerprint, but we already + # have everything we need to see if our seed is actually in + # the output script. + for derivation_path_obj in out.bip32_derivations.values(): + # Each entry pairs a derivation path with the public key + # the coordinator says sits there. Both are its own + # claims, so we read only the path and derive the key + # ourselves. + seed_public_key = PSBTParser._derive_with_cache(self.root, derivation_path_obj.derivation, child_key_derivation_cache).get_public_key() + + if PSBTParser._multisig_script_contains_key(multisig_script, seed_public_key): + # The output pays a multisig this seed is part + # of, but the psbt did not claim our key there. + # We treat this deception as an attack. + raise PSBTOutputOwnershipContradictionError(f"Output's committed script holds this seed's key at {bip32.path_to_str(derivation_path_obj.derivation)} but the psbt claims another fingerprint and/or public key there") + + # Every path the psbt supplied has been checked and none puts + # this seed in the committed script, so the output is an + # external spend. + is_change = False + + else: + # This output claimed that our seed is part of the receiving + # multisig, at a specific path. So now we verify that the key + # at that path is in the committed script. + seed_public_key = PSBTParser._derive_with_cache(self.root, verified_derivation_path, child_key_derivation_cache).get_public_key() + if not PSBTParser._multisig_script_contains_key(multisig_script, seed_public_key): + # The psbt said this output was coming back to our seed + # at that path, but the key there is not in the committed + # script. We treat this deception as an attack. + raise PSBTOutputOwnershipContradictionError(f"Output claims this seed at {bip32.path_to_str(verified_derivation_path)} but its committed script does not hold that key") + + # The output should not describe more keys than are actually + # used in its script. We check for the more serious deceptions + # before this so they can be surfaced first. + if len(out.bip32_derivations) > self.policy["n"]: + # We don't try to decide if this is an attack or a + # mistake. We just abort the parse. + raise PSBTSurplusDerivationPathsError("Multisig output claims more derivation paths than its script has keys") + + else: + # No handler claimed a matching output, which the branches above + # should make impossible. Raise rather than leave is_change True; + # that would record change with nothing verified behind it. + raise RuntimeError(f"Output {i} matched but no verification handler applies") + + elif verified_derivation_path is not None and "p2tr" not in self.policy["type"]: + # The psbt claims one of this seed's keys on this output, yet the + # output does NOT pay what that claim describes. We treat this + # deception as an attack. + # * single sig: verified that this output is not paying our seed at + # the claimed derivation path. + # * multisig: verified that the output's claimed script is not the + # one the output commits to. Note that we haven't verified our + # seed's participation in the claimed script; it's irrelevant if + # that script isn't committed to in the scriptPubKey. + # Taproot is exempt: an output paying our internal key tweaked by + # a script tree fails the rebuild above even when the psbt claimed + # the seed truthfully, and from here that is indistinguishable + # from an output that claims our key and pays someone else. + # TODO: Parse PSBT_OUT_TAP_TREE, which embit leaves unparsed in + # the scope's `unknown` map. Its merkle root is what separates the + # two: a tree that tweaks our key to the committed key makes the + # output verifiable change, one that does not is a contradiction to + # refuse here, and an output supplying no tree stays exempt, since + # an omitted optional field is not a contradiction. + raise PSBTOutputOwnershipContradictionError(f"Output claims this seed at {bip32.path_to_str(verified_derivation_path)} but its committed script contradicts that") + if vout[i].script_pubkey.data[0] == OPCODES.OP_RETURN: # The data is written as: OP_RETURN + OP_PUSHDATA1 + len(payload) + payload self.op_return_data = vout[i].script_pubkey.data[3:] elif is_change: addr = vout[i].script_pubkey.address(NETWORKS[SettingsConstants.map_network_to_embit(self.network)]) - claimed_fingerprints = [] - claimed_derivation_paths = [] - - # extract info from non-taproot outputs - if len(self.psbt.outputs[i].bip32_derivations) > 0: - for d, derivation_path in self.psbt.outputs[i].bip32_derivations.items(): - claimed_fingerprints.append(hexlify(derivation_path.fingerprint).decode()) - claimed_derivation_paths.append(bip32.path_to_str(derivation_path.derivation)) - - # extract info from taproot outputs - if len(self.psbt.outputs[i].taproot_bip32_derivations) > 0: - for d, (leaf_hashes, derivation) in self.psbt.outputs[i].taproot_bip32_derivations.items(): - claimed_fingerprints.append(hexlify(derivation.fingerprint).decode()) - claimed_derivation_paths.append(bip32.path_to_str(derivation.derivation)) - self.change_data.append({ "output_index": i, "address": addr, "amount": vout[i].value, - "claimed_fingerprints": claimed_fingerprints, - "claimed_derivation_paths": claimed_derivation_paths, + "verified_derivation_path": self.verified_output_derivation_paths[i], }) self.change_amount += vout[i].value @@ -436,7 +646,7 @@ class PSBTParser(): if script is not None: m, n, pubkeys = PSBTParser._parse_multisig(script) - + # check pubkeys are derived from cosigners try: cosigners = PSBTParser._get_cosigners(pubkeys, scope.bip32_derivations, xpubs, child_key_derivation_cache) @@ -445,14 +655,55 @@ class PSBTParser(): # TODO: stop swallowing everything here. This also catches bugs in the # cosigner check itself, and cannot tell those apart from the psbt # simply not supplying xpubs to check against, which is valid and must - # not be rejected outright. The fallback policy carries no cosigner - # information at all, and two of those compare equal on script type - # and m-of-n alone. Fix pending with the multisig verification work. + # not be rejected outright. policy.update({"m": m, "n": n}) - + return policy + @staticmethod + def _policy_shape_matches(policy_a: dict, policy_b: dict) -> bool: + """ + Compares two policies on the shape of the script they describe: the script type, + plus m-of-n for multisig. + + A policy can also carry the cosigners resolved from the coordinator's global + xpubs. Those are never authoritative here, and comparing them would let a psbt + decide which of its own outputs get verified: one misannotated fingerprint makes + that output's cosigners fail to resolve, and the output then stops matching the + inputs' policy. Shape comes from the scriptPubKey and the supplied script, and the + caller proves ownership rather than assuming it. + """ + for field in ("type", "m", "n"): + if policy_a.get(field) != policy_b.get(field): + return False + + return True + + + @staticmethod + def _build_singlesig_script(policy_type: str, public_key: PublicKey) -> script.Script: + """ + Builds the scriptPubKey that pays public_key under the given single-sig + policy_type. + """ + if policy_type == "p2pkh": + return script.p2pkh(public_key) + + if policy_type == "p2sh-p2wpkh": + return script.p2sh(script.p2wpkh(public_key)) + + if policy_type == "p2wpkh": + return script.p2wpkh(public_key) + + if policy_type == "p2tr": + return script.p2tr(public_key) + + # Shouldn't be able to reach here. Just a guard against a future developer calling + # this with invalid args. + raise RuntimeError(f"Not a single-sig script type: {policy_type}") + + @staticmethod def _parse_multisig(multisig_script): """Takes a script and extracts m,n and pubkeys from it""" @@ -482,6 +733,15 @@ class PSBTParser(): return m, n, pubkeys + @staticmethod + def _multisig_script_contains_key(multisig_script: script.Script, public_key: PublicKey) -> bool: + """ + Determines whether multisig_script includes the provided public_key. + """ + m, n, pubkeys = PSBTParser._parse_multisig(multisig_script) + return any(pubkey.sec() == public_key.sec() for pubkey in pubkeys) + + @staticmethod def _derive_with_cache(parent_key: bip32.HDKey, derivation_path: List[int], child_key_derivation_cache: dict | None = None) -> bip32.HDKey: """ @@ -576,7 +836,7 @@ class PSBTParser(): for pub, (leaf_hashes, derivation_path) in input.taproot_bip32_derivations.items(): # TODO: Support spends from leaves; depends on support in embit if len(leaf_hashes) > 0: - raise Exception("Signing keyspends from within a taptree not yet implemented") + raise Exception("Signing script path spends is not yet implemented") fingerprints.add(hexlify(derivation_path.fingerprint).decode()) return list(fingerprints) @@ -593,7 +853,7 @@ class PSBTParser(): instance. """ seed_fingerprint = seed.get_fingerprint(network) - + def check_fingerprint_match(public_key: PublicKey, derivation_path_obj: DerivationPath, is_taproot: bool): """Check fingerprint match with missing fingerprint fallback""" @@ -621,7 +881,7 @@ class PSBTParser(): for public_key, (leaf_hashes, derivation_path_obj) in input.taproot_bip32_derivations.items(): if check_fingerprint_match(public_key, derivation_path_obj, is_taproot=True): return True - + return False @@ -667,6 +927,15 @@ class PSBTParser(): Every key in the scope that claims this seed's fingerprint is re-derived and checked. A false claim raises PSBT[Output|Input]OwnershipClaimError. + A further check is then enforced: a scope carrying entries in the + bip32_derivations AND taproot_bip32_derivations maps raises + PSBTMixedDerivationPathTypesError. Taproot keys are exclusively x-only while + non-taproot keys always carry their parity byte; there is no script type that can + make use of both types of keys so it is nonsensical for a scope to provide both. + + Note that neither BIP-174 nor BIP-371 forbids the combination. And embit will + parse and even sign such a psbt. We disallow it by opinionated choice. + One edge case: * A multisig could use this seed in more than one cosigner slot, each at its own derivation path. The scope then carries several entries that all @@ -700,17 +969,23 @@ class PSBTParser(): _check_claim(public_key, derivation_path_obj, is_taproot=False) for public_key, (leaf_hashes, derivation_path_obj) in scope.taproot_bip32_derivations.items(): - # TODO: Support keys in taptree leaves + # TODO: Support keys in script tree leaves _check_claim(public_key, derivation_path_obj, is_taproot=True) + # The derivation path maps cannot both be populated in the same scope. Checked + # after the loops so that a false ownership claim, the more serious finding, is + # still the one reported. + if scope.bip32_derivations and scope.taproot_bip32_derivations: + raise PSBTMixedDerivationPathTypesError("Scope declares both ecdsa and taproot derivation paths") + return verified_derivation_path def _verify_claimed_derivation_paths(self, child_key_derivation_cache: dict): """ - Verifies every claimed derivation path that names this seed's fingerprint. The + Verifies every derivation path entry that claims this seed's fingerprint. The result, stored in verified_[input|output]_derivation_paths, is either the verified - derivation path or None (the seed was not named) for each input/output scope. + derivation path or None (no entry claimed this seed) for each input/output scope. The coordinator-supplied fingerprints cannot be trusted as-is. We must derive and verify the ownership of each one that claims to belong to this seed. @@ -749,7 +1024,7 @@ class PSBTParser(): harmless: the excluded key cannot spend the input, so nothing of this seed's is at risk.) """ - # An input names a key at a derivation path and _verify_claimed_derivation_paths + # An input claims a key at a derivation path and _verify_claimed_derivation_paths # proved the seed derives it (single-sig: one such key; multisig: one per # cosigner, ours among them). One verified input path is enough for the psbt to # be signable. @@ -760,6 +1035,15 @@ class PSBTParser(): raise PSBTSeedCannotSignError() + @staticmethod + def is_change_branch(derivation_path: List[int]) -> bool: + """ + Returns True if the next-to-last element of the derivation path is the change + branch (1). + """ + return len(derivation_path) >= 2 and derivation_path[-2] == 1 + + def verify_multisig_output(self, descriptor: Descriptor, change_num: int) -> bool: change_data = self.get_change_data(change_num) i = change_data["output_index"] diff --git a/src/seedsigner/views/psbt_views.py b/src/seedsigner/views/psbt_views.py index 77dbb356..dba57fb4 100644 --- a/src/seedsigner/views/psbt_views.py +++ b/src/seedsigner/views/psbt_views.py @@ -1,11 +1,13 @@ from gettext import gettext as _ from seedsigner.models.psbt_parser import (PSBTInputOwnershipClaimError, - PSBTOutputOwnershipClaimError, PSBTParser, PSBTSeedCannotSignError) + PSBTMixedDerivationPathTypesError, PSBTOutputOwnershipClaimError, + PSBTOutputOwnershipContradictionError, PSBTParser, PSBTSeedCannotSignError, + PSBTSurplusDerivationPathsError) from seedsigner.models.settings import SettingsConstants from seedsigner.gui.components import FontAwesomeIconConstants, GUIConstants, SeedSignerIconConstants from seedsigner.gui.screens.screen import (RET_CODE__BACK_BUTTON, ButtonListScreen, ButtonOption, LargeIconStatusScreen, WarningScreen, DireWarningScreen, QRDisplayScreen) -from seedsigner.views.view import BackStackView, MainMenuView, NotYetImplementedView, View, Destination +from seedsigner.views.view import BackStackView, MainMenuView, View, Destination @@ -103,16 +105,28 @@ class PSBTOverviewView(View): network=self.settings.get_value(SettingsConstants.SETTING__NETWORK) ) + # Note that in almost every exception case, we set clear_history to disable + # returning via BACK button in the Destination. except PSBTInputOwnershipClaimError: - # Set clear_history to disable returning via BACK button self.set_redirect(Destination(PSBTInputOwnershipClaimFailedView, clear_history=True)) return except PSBTOutputOwnershipClaimError: - # Set clear_history to disable returning via BACK button self.set_redirect(Destination(PSBTOutputOwnershipClaimFailedView, clear_history=True)) return + except PSBTSurplusDerivationPathsError: + self.set_redirect(Destination(PSBTSurplusDerivationPathsView, clear_history=True)) + return + + except PSBTMixedDerivationPathTypesError: + self.set_redirect(Destination(PSBTMixedDerivationPathTypesView, clear_history=True)) + return + + except PSBTOutputOwnershipContradictionError: + self.set_redirect(Destination(PSBTOutputOwnershipContradictionView, clear_history=True)) + return + except PSBTSeedCannotSignError: # Not a suspicious psbt, just the wrong seed for it. Send the user back to # pick another rather than clearing the flow. @@ -133,17 +147,18 @@ class PSBTOverviewView(View): """ change_data = [ { + 'output_index': 0, 'address': 'bc1q............', 'amount': 397621401, - 'claimed_fingerprints': ['22bde1a9', '73c5da0a'], - 'claimed_derivation_paths': ['m/48h/1h/0h/2h/1/0', 'm/48h/1h/0h/2h/1/0'] + 'verified_derivation_path': + [2147483696, 2147483649, 2147483648, 2147483650, 1, 0], }, {}, ] """ num_change_outputs = 0 num_self_transfer_outputs = 0 for change_output in change_data: - if change_output["claimed_derivation_paths"][0].split("/")[-2] == "1": + if PSBTParser.is_change_branch(change_output["verified_derivation_path"]): num_change_outputs += 1 else: num_self_transfer_outputs += 1 @@ -325,6 +340,7 @@ class PSBTChangeDetailsView(View): def run(self): + from embit import bip32 from seedsigner.gui.screens.psbt_screens import PSBTChangeDetailsScreen psbt_parser: PSBTParser = self.controller.psbt_parser @@ -332,32 +348,24 @@ class PSBTChangeDetailsView(View): # Should not be able to get here return Destination(MainMenuView) - # Can we verify this change addr? change_data = psbt_parser.get_change_data(change_num=self.change_address_num) """ change_data: { + 'output_index': 0, 'address': 'bc1q............', 'amount': 397621401, - 'claimed_fingerprints': ['22bde1a9', '73c5da0a'], - 'claimed_derivation_paths': ['m/48h/1h/0h/2h/1/0', 'm/48h/1h/0h/2h/1/0'] + 'verified_derivation_path': + [2147483696, 2147483649, 2147483648, 2147483650, 1, 0], } """ - - # Single-sig verification is easy. We expect to find a single fingerprint - # and derivation path. seed_fingerprint = self.controller.psbt_seed.get_fingerprint(self.settings.get_value(SettingsConstants.SETTING__NETWORK)) + verified_derivation_path = change_data.get("verified_derivation_path") + is_change_derivation_path = PSBTParser.is_change_branch(verified_derivation_path) - if seed_fingerprint not in change_data.get("claimed_fingerprints"): - # TODO: Something is wrong with this psbt(?). Reroute to warning? - return Destination(NotYetImplementedView) - - i = change_data.get("claimed_fingerprints").index(seed_fingerprint) - claimed_derivation_path = change_data.get("claimed_derivation_paths")[i] - - # 'm/84h/1h/0h/1/0' would be a change addr while 'm/84h/1h/0h/0/0' is a self-receive - is_change_derivation_path = int(claimed_derivation_path.split("/")[-2]) == 1 - derivation_path_addr_index = int(claimed_derivation_path.split("/")[-1]) + # TODO: Refuse a path too short to carry a branch and an address index; until then + # this can raise on a malformed psbt. + derivation_path_addr_index = verified_derivation_path[-1] if is_change_derivation_path: # TRANSLATOR_NOTE: The amount you're receiving back from the transaction @@ -370,66 +378,33 @@ class PSBTChangeDetailsView(View): is_change_addr_verified = False if psbt_parser.is_multisig: + # Multisig is verified here rather than during the initial parse because the + # descriptor it needs to be checked against does not arrive until mid-flow. + # TODO: Verify the multisig change as soon as the descriptor is loaded, rather + # than waiting to do it here. + # if the known-good multisig descriptor is already onboard: if self.controller.multisig_wallet_descriptor: is_change_addr_verified = psbt_parser.verify_multisig_output(self.controller.multisig_wallet_descriptor, change_num=self.change_address_num) button_data = [self.NEXT] else: - # Have the Screen offer to load in the multisig descriptor. + # Nothing to check against yet. Have the Screen offer to load in the + # multisig descriptor. button_data = [self.VERIFY_MULTISIG, self.SKIP_VERIFICATION] else: - # Single sig - try: - from embit import script - from embit.networks import NETWORKS + # The PSBTParser already proves that single sig change outputs are owned by + # this seed. + is_change_addr_verified = True + button_data = [self.NEXT] - if is_change_derivation_path: - loading_screen_text = _("Verifying Change...") - else: - loading_screen_text = _("Verifying Self-Transfer...") - from seedsigner.gui.screens.screen import LoadingScreenThread - loading_screen = LoadingScreenThread(text=loading_screen_text) - loading_screen.start() - - # convert change address to script pubkey to get script type - pubkey = script.address_to_scriptpubkey(change_data["address"]) - script_type = pubkey.script_type() - - # extract derivation path to get wallet and change derivation - change_path = '/'.join(claimed_derivation_path.split("/")[-2:]) - wallet_path = '/'.join(claimed_derivation_path.split("/")[:-2]) - - xpub = self.controller.psbt_seed.get_xpub( - wallet_path=wallet_path, - network=self.settings.get_value(SettingsConstants.SETTING__NETWORK) - ) - - # take script type and call script method to generate address from seed / derivation - xpub_key = xpub.derive(change_path).key - network = self.settings.get_value(SettingsConstants.SETTING__NETWORK) - scriptcall = getattr(script, script_type) - if script_type == "p2sh": - # single sig only so p2sh is always p2sh-p2wpkh - calc_address = script.p2sh(script.p2wpkh(xpub_key)).address( - network=NETWORKS[SettingsConstants.map_network_to_embit(network)] - ) - else: - # single sig so this handles p2wpkh and p2wpkh (and p2tr in the future) - calc_address = scriptcall(xpub_key).address( - network=NETWORKS[SettingsConstants.map_network_to_embit(network)] - ) - - if change_data["address"] == calc_address: - is_change_addr_verified = True - button_data = [self.NEXT] - - finally: - loading_screen.stop() - - if is_change_addr_verified == False and (not psbt_parser.is_multisig or self.controller.multisig_wallet_descriptor is not None): - return Destination(PSBTAddressVerificationFailedView, view_args=dict(is_change=is_change_derivation_path, is_multisig=psbt_parser.is_multisig), clear_history=True) + # TODO: Will be unnecessary once the above update is made to verify multisig + # change as soon as the descriptor is loaded. + if not is_change_addr_verified and self.controller.multisig_wallet_descriptor is not None: + # Verification failed, so this psbt is done. + # Set clear_history to disable returning via BACK button. + return Destination(PSBTAddressVerificationFailedView, view_args=dict(is_change=is_change_derivation_path), clear_history=True) selected_menu_num = self.run_screen( PSBTChangeDetailsScreen, @@ -439,7 +414,7 @@ class PSBTChangeDetailsView(View): amount=change_data.get("amount"), is_multisig=psbt_parser.is_multisig, fingerprint=seed_fingerprint, - derivation_path=claimed_derivation_path, + derivation_path=bip32.path_to_str(verified_derivation_path), is_change_derivation_path=is_change_derivation_path, derivation_path_addr_index=derivation_path_addr_index, is_change_addr_verified=is_change_addr_verified, @@ -470,7 +445,10 @@ class PSBTChangeDetailsView(View): class PSBTSeedCannotSignView(View): """ Reached when parsing found this seed can't sign any of the psbt's inputs (see - PSBTSeedCannotSignError). Routes back to seed selection so the user can pick another. + PSBTSeedCannotSignError). + + We do not view this as an attack; most likely the user simply selected the wrong seed. + Routes back to seed selection rather than discarding the psbt. """ SELECT_DIFFERENT_SEED = ButtonOption("Select a different seed") @@ -497,8 +475,11 @@ class PSBTSeedCannotSignView(View): class PSBTOutputOwnershipClaimFailedView(View): """ Reached when a false ownership claim on an output rejects the psbt (see - PSBTOutputOwnershipClaimError). Shows a dire warning and discards the psbt to the main - menu. Claims on inputs route to PSBTInputOwnershipClaimFailedView instead. + PSBTOutputOwnershipClaimError). Claims on inputs route to + PSBTInputOwnershipClaimFailedView instead. + + We view this as an attack. We do not allow the user to continue and give this the + "Dire Warning" level. """ DISCARD = ButtonOption("Discard transaction") @@ -507,13 +488,13 @@ class PSBTOutputOwnershipClaimFailedView(View): DireWarningScreen, title=_("Suspicious Transaction"), status_headline=_("Likely an Attack!"), - text=_("The transaction's change/self-transfer outputs are not going back to your wallet."), + text=_("The transaction's change/self-transfer output is not going back to your wallet."), button_data=[self.DISCARD], show_back_button=False, ) - # We're done with this PSBT. Route back to MainMenuView, which clears all ephemeral - # data (except in-memory seeds). + # We're done with this PSBT. Route back to MainMenuView, which clears all + # ephemeral data (except in-memory seeds). # Set clear_history to disable returning via BACK button. return Destination(MainMenuView, clear_history=True) @@ -522,8 +503,11 @@ class PSBTOutputOwnershipClaimFailedView(View): class PSBTInputOwnershipClaimFailedView(View): """ Reached when a false ownership claim on an input rejects the psbt (see - PSBTInputOwnershipClaimError). Shows a plain (not dire) warning and discards the psbt - to the main menu. + PSBTInputOwnershipClaimError). + + We do not view this as an attack; a forged input claim only renders the psbt + unsignable. We do not allow the user to continue, but only give this the "Warning" + level. """ DISCARD = ButtonOption("Discard transaction") @@ -532,13 +516,99 @@ class PSBTInputOwnershipClaimFailedView(View): WarningScreen, title=_("Transaction Problem"), status_headline=None, - text=_("This transaction incorrectly claims that its input(s) belong to this seed."), + text=_("This transaction incorrectly claims that one of its inputs belongs to this seed."), button_data=[self.DISCARD], show_back_button=False, ) - # We're done with this PSBT. Route back to MainMenuView, which clears all ephemeral - # data (except in-memory seeds). + # We're done with this PSBT. Route back to MainMenuView, which clears all + # ephemeral data (except in-memory seeds). + # Set clear_history to disable returning via BACK button. + return Destination(MainMenuView, clear_history=True) + + + +class PSBTSurplusDerivationPathsView(View): + """ + Reached when an output claims more derivation path entries than its script can use + (see PSBTSurplusDerivationPathsError). + + The single sig case is a structural error. The multisig case could be an attempt to + deceive but since we don't know for sure, it's sufficient to just use the "Warning" + level and stop the user from continuing. + """ + DISCARD = ButtonOption("Discard transaction") + + def run(self): + self.run_screen( + WarningScreen, + title=_("Transaction Problem"), + status_headline=None, + # TRANSLATOR_NOTE: The transaction/psbt has an error but does not seem to be malicious. + text=_("This transaction claims too many keys for one of its outputs."), + button_data=[self.DISCARD], + show_back_button=False, + ) + + # We're done with this PSBT. Route back to MainMenuView, which clears all + # ephemeral data (except in-memory seeds). + # Set clear_history to disable returning via BACK button. + return Destination(MainMenuView, clear_history=True) + + + +class PSBTMixedDerivationPathTypesView(View): + """ + Reached when an input or output describes its keys in both derivation path maps at + once (PSBTMixedDerivationPathTypesError). + + We view this as a strange / buggy psbt and do not try to decide whether it is + malicious. We do not allow the user to continue, but only give this the "Warning" + level. + """ + DISCARD = ButtonOption("Discard transaction") + + def run(self): + self.run_screen( + WarningScreen, + title=_("Transaction Problem"), + status_headline=None, + # TRANSLATOR_NOTE: The transaction/psbt has an error but does not seem to be malicious. + text=_("This transaction claims taproot and non-taproot keys for the same script."), + button_data=[self.DISCARD], + show_back_button=False, + ) + + # We're done with this PSBT. Route back to MainMenuView, which clears all + # ephemeral data (except in-memory seeds). + # Set clear_history to disable returning via BACK button. + return Destination(MainMenuView, clear_history=True) + + + +class PSBTOutputOwnershipContradictionView(View): + """ + Reached when the psbt's account of who an output pays contradicts the script that + output commits to (see PSBTOutputOwnershipContradictionError). + + We view this as an attack. We do not allow the user to continue and give this the + "Dire Warning" level. + """ + DISCARD = ButtonOption("Discard transaction") + + def run(self): + self.run_screen( + DireWarningScreen, + title=_("Suspicious Transaction"), + status_headline=_("Likely an Attack!"), + # TRANSLATOR_NOTE: The transaction/psbt contains a deception that we consider an attack. + text=_("This transaction misrepresents where one of its outputs pays."), + button_data=[self.DISCARD], + show_back_button=False, + ) + + # We're done with this PSBT. Route back to MainMenuView, which clears all + # ephemeral data (except in-memory seeds). # Set clear_history to disable returning via BACK button. return Destination(MainMenuView, clear_history=True) @@ -546,23 +616,22 @@ class PSBTInputOwnershipClaimFailedView(View): class PSBTAddressVerificationFailedView(View): """ - Reached when a change or self-transfer output fails address verification. Shows a dire - warning and discards the psbt to the main menu. + Reached from PSBTChangeDetailsView when a multisig change or self-transfer output + could not be verified against the descriptor the user supplied. + + We view this as suspicious but stop short of calling it an attack, since the user may + have loaded the wrong wallet's descriptor. We do not allow the user to continue and + give this the "Dire Warning" level. """ - def __init__(self, is_change: bool = True, is_multisig: bool = False): + def __init__(self, is_change: bool = True): super().__init__() self.is_change = is_change - self.is_multisig = is_multisig def run(self): - if self.is_multisig: - # TRANSLATOR_NOTE: Variable is either "change" or "self-transfer". - text = _("Transaction's {} address could not be verified from wallet descriptor.").format(_("change") if self.is_change else _("self-transfer")) - else: - # TRANSLATOR_NOTE: Variable is either "change" or "self-transfer". - text = _("Transaction's {} address could not be generated from your seed.").format(_("change") if self.is_change else _("self-transfer")) - + # TRANSLATOR_NOTE: Variable is either "change" or "self-transfer". + text = _("Transaction's {} address could not be verified from wallet descriptor.").format(_("change") if self.is_change else _("self-transfer")) + self.run_screen( DireWarningScreen, title=_("Suspicious Transaction"), @@ -572,8 +641,8 @@ class PSBTAddressVerificationFailedView(View): show_back_button=False, ) - # We're done with this PSBT. Route back to MainMenuView, which clears all ephemeral - # data (except in-memory seeds). + # We're done with this PSBT. Route back to MainMenuView, which clears all + # ephemeral data (except in-memory seeds). # Set clear_history to disable returning via BACK button. return Destination(MainMenuView, clear_history=True) @@ -661,8 +730,8 @@ class PSBTSignedQRDisplayView(View): ) self.run_screen(QRDisplayScreen, qr_encoder=qr_encoder) - # We're done with this PSBT. Route back to MainMenuView which always - # clears all ephemeral data (except in-memory seeds). + # We're done with this PSBT. Route back to MainMenuView, which clears all + # ephemeral data (except in-memory seeds). return Destination(MainMenuView, clear_history=True) diff --git a/tests/psbt_testing_util.py b/tests/psbt_testing_util.py index 00ea93d2..85ebe827 100644 --- a/tests/psbt_testing_util.py +++ b/tests/psbt_testing_util.py @@ -1,8 +1,9 @@ from binascii import a2b_base64, unhexlify from io import BytesIO -from embit import bip32 +from embit import bip32, script from embit.ec import PublicKey +from embit.hashes import tagged_hash from embit.networks import NETWORKS from embit.psbt import PSBT, DerivationPath, InputScope, OutputScope @@ -27,6 +28,41 @@ class PSBTTestData: MULTISIG_NESTED_SEGWIT_1_INPUT = "cHNidP8BADMCAAAAAbtbjTfAfTR/t88dfkGmcZBz0l/uzrLuJEyf0WMHl94vAQAAAAD9////AHEAAABPAQQ1h88EmoSnUYAAAAF+MfPe6kyaEEX91G1HKwksGaixXN6RMBTxIcHKjNYYoQJBzkFqUsm3ttoRsGfx+CMj/77pmyyjitqjWZPG4d3RrxQPuIL/MAAAgAEAAIAAAACAAQAAgE8BBDWHzwRA+hbPgAAAAcRAG6PxSUPbFp8IvKVuNlIQn4W5TK1ceLGdKkxdSfktA8QjWNuOjADhspq0onHkGp117FftE91lAYTev8snQsNRFAPNCiswAACAAQAAgAAAAIABAACATwEENYfPBMtS+WKAAAAB6inmZ+P+TS/JJhI/Fog5q8Rx2Nik9EJVEugTuPSDcI4CbvN4qeJalXMOlJpoKnL9Y64icQtz01M9NG6SOhcG8uQUD4iQRDAAAIABAACAAAAAgAEAAIAAAQBzAgAAAAHCfPUdyeETNxf9pkzBP2oOUfgSISrrZi4uj0boKN2PeQEAAAAA/f///wKwqEIGAQAAABepFOV0lUEDO1EMcpnbm8u5gOS//denhwDh9QUAAAAAF6kUURslNGksU91RDOgpxCM7IQOoQaSHAAAAAAEBIADh9QUAAAAAF6kUURslNGksU91RDOgpxCM7IQOoQaSHAQMEAQAAAAEEIgAgDHlipQ8OV+Wko64bycNh+v4LfTW5d8cTv3T1n4XDS/sBBWlSIQIsxclipQM/Gs3kdO+Mlg1gbcMRf1ukSklJhhQ8iMXpSSEDjvwJUpUWl14h2ma/DH5VeAEhM9PxTz70b6lOAgDiWrshA8FwdXOWiPDUIOtV4aCz/ZxfLr9fwrpHQLCUeAtGctJiU64iBgOO/AlSlRaXXiHaZr8MflV4ASEz0/FPPvRvqU4CAOJauxwPuIL/MAAAgAEAAIAAAACAAQAAgAAAAAAAAAAAIgYCLMXJYqUDPxrN5HTvjJYNYG3DEX9bpEpJSYYUPIjF6UkcA80KKzAAAIABAACAAAAAgAEAAIAAAAAAAAAAACIGA8FwdXOWiPDUIOtV4aCz/ZxfLr9fwrpHQLCUeAtGctJiHA+IkEQwAACAAQAAgAAAAIABAACAAAAAAAAAAAAA" MULTISIG_LEGACY_P2SH_1_INPUT = "cHNidP8BADMCAAAAAarS4QMzScnPvNBT2B1I3h80UrSoNuKd9vZkjSl7j2WKAQAAAAD9////AHQAAABPAQQ1h88BD7iC/4AAAC16oZcoUbg2ksYGYWMICvKISPS51jTZLJ3tu4schhGxcAJ/o4LdLxGZHyVDceUz5n6ZtABk9X6nBk3yz/f+eXxkpQgPuIL/LQAAgE8BBDWHzwEDzQorgAAALaD15sHGuMCJCZiW09JfCCZEKGcn9WtB395d/8vj/WC7Am982IdFkEBytDXikxzoeLV5q3kpHg+bfmsmj7ncr1lgCAPNCistAACATwEENYfPAQ+IkESAAAAtcUAwLhCloJPpswRwhGdyG3KP1kY0VAetU6FdAmNrqQoDKFgD/CXYCi5ZHqx4HImYJZtoxpjx60Ki7kvK4Ean8FkID4iQRC0AAIAAAQBzAgAAAAFOE/9Gl/ZpjWR8ioftAB2GPhLXScZ3TEa2tIJ+EDwmXQAAAAAA/f///wI+TSQYAQAAABepFE+Jek+WpV0vCjnVpkLqKfoiLvPAhwDh9QUAAAAAF6kUdHD4u+bbshVEAVKkqRQxu+KR8FKHaAAAAAEDBAEAAAABBGlSIQLUf/ihLNEohwIQiCxVNGmPROLzi8t9FnV6DmfCuEhXHCEDK1QlMflvscOGZmSDWJi90FPUQvNFTQUbkkRdowbR0RghA2T1w+njATChE10XcsbguMj0J5RIzJ5TVN7sbVckbzUWU64iBgLUf/ihLNEohwIQiCxVNGmPROLzi8t9FnV6DmfCuEhXHBAPuIL/LQAAgAAAAAAAAAAAIgYDZPXD6eMBMKETXRdyxuC4yPQnlEjMnlNU3uxtVyRvNRYQA80KKy0AAIAAAAAAAAAAACIGAytUJTH5b7HDhmZkg1iYvdBT1ELzRU0FG5JEXaMG0dEYEA+IkEQtAACAAAAAAAAAAAAA" + # Unlike the fixtures above, this is a complete psbt: its outputs are intact and it + # has its own wallet. Use it wherever a test needs more than one input, or needs + # inputs and change at known, differing address indices. + # + # PSBT Tx and Wallet Details + # - Single Sig Wallet P2WPKH (Native Segwit) with no passphrase + # - Regtest 394aed14 m/84'/1'/0' tpubDC4rddHCzuinGFEKhiD8A3Ku5GRcNVAz3BitfwL4wn7zKPCya4i2CZLX8uoP8oCdC8VEe4jXb5u9ePCSrRA68YRSgBQra5YzBajyxKFDz4C + # - 2 Inputs + # - 56,522,834 sats at 1/6 + # - 1,990,245,069 sats at 1/0 + # - 4 Outputs + # - 1 Output spend to another wallet: 123,456 sats to bcrt1q7cw0wzy8g6mq5qvkpvhnk5gsps5ncy3srp0n2j + # - 3 Outputs change + # - 1/7 address bcrt1q53j0xwuskuf5gnvynadh0hlazyy8srydlucrhg with amount 123,456 sats + # - 1/8 address bcrt1q5gtw3zfp4cx67yk5q42q6j6rfza8aqcwpyyslv with amount 56,399,242 sats + # - 1/9 address bcrt1q9rrg7399m43cn0yg4tz0v0ate89jgf2d6kpz7v with amount 1,990,121,477 sats + # - Fee 272 sats + two_input_seed = Seed("goddess rough corn exclude cream trial fee trumpet million prevent gaze power".split()) + SINGLE_SIG_NATIVE_SEGWIT_2_INPUTS = "cHNidP8BANgCAAAAAsTXZs3fz/dmGb6M80+jjvJZdYya+cw5bT/dGuhZFdSlAAAAAAD9////qo6xg/UZAvUkcbse1F+C9zbP/FeZNjThx7SCIn6eMCgBAAAAAP3///8EQOIBAAAAAAAWABSkZPM7kLcTRE2En1t33/0RCHgMjQXYnnYAAAAAFgAUKMaPRKXdY4m8iKrE9j+rycskJU1A4gEAAAAAABYAFPYc9wiHRrYKAZYLLztREAwpPBIwipVcAwAAAAAWABSiFuiJIa4NrxLUBVQNS0NIun6DDtoRAABPAQQ1h88DBcQGZIAAAAA+0J+jlNL3dpWwlnBi8Dx+Ipg4e6uvB3HdjzFPX7r9CAOOlAIxgII+/xCcj+XoEenKH7wj5s5wlu7Q7CCZWFLGLhA5Su0UVAAAgAEAAIAAAACAAAEA7QIAAAAEE6njX/fnvn7hbkKIRcxzNYFOSfbCdNeWnd7Fe/1UcQ0BAAAAAP3///8TqeNf9+e+fuFuQohFzHM1gU5J9sJ015ad3sV7/VRxDQMAAAAA/f///xOp41/3575+4W5CiEXMczWBTkn2wnTXlp3exXv9VHENBAAAAAD9////E6njX/fnvn7hbkKIRcxzNYFOSfbCdNeWnd7Fe/1UcQ0GAAAAAP3///8CUnheAwAAAAAWABRCfygPJ+Fjsx4BknYvvm3A3qKn2xJ/XQcAAAAAF6kU1I4TAst5nAj15ey7vwe5cM3OFq+HlhEAAAEBH1J4XgMAAAAAFgAUQn8oDyfhY7MeAZJ2L75twN6ip9sBAwQBAAAAIgYCo7sfm78RQY3B5n0ac/QF8VtMAzFnci+h5D1MtpgRY7oYOUrtFFQAAIABAACAAAAAgAEAAAAGAAAAAAEAcQIAAAABxY7wh0nsfJQfzWrD/9rN9BYsM+iOmPaO6I0ANFgO/PcAAAAAAP3///8CptiUAAAAAAAWABRIm4HhQY/TzOjeWSPRrbuJo9MlW826oHYAAAAAFgAU0z+0L2QSLGtyQTn8FhbCpcI7jbliAQAAAQEfzbqgdgAAAAAWABTTP7QvZBIsa3JBOfwWFsKlwjuNuQEDBAEAAAAiBgITHmebEANk81CraV4xZIpqkNjjw0tIvezl1Ism1NRH3Rg5Su0UVAAAgAEAAIAAAACAAQAAAAAAAAAAIgICuTT7WnuiUTpObjWnZFHzIeEvW9PTB+1LLVFNQJVFeIIYOUrtFFQAAIABAACAAAAAgAEAAAAHAAAAACICAk8f3hpc5C35chgSg+Pe2zZ9IhHREd4aKW2+yAMRIFeqGDlK7RRUAACAAQAAgAAAAIABAAAACQAAAAAAIgIDjt1CjvrnMMnjbmTNKUAYoKEDRbmKjNjbq+6Ppqj3bqQYOUrtFFQAAIABAACAAAAAgAEAAAAIAAAAAA==" + + # Also complete and on the same wallet as the fixture above. Taproot records its + # derivation paths in a separate map from the segwit-v0 one, so this is what tests of + # that split need. + # + # PSBT Tx and Wallet Details + # - Single Sig Wallet P2TR (Taproot) with no passphrase + # - Regtest 394aed14 m/86'/1'/0' tpubDCawGrRg7YdHdFb9p4mmD8GBaZjJegL53FPFRrMkGoLcgLATJfksUs2y1Q7dVzixAkgecazsxEsUuyj3LyDw7eVVYHQyojwrc2hfesK4wXW + # - 1 Input + # - 3,190,493,401 sats at 1/0 + # - 2 Outputs + # - 1 Output change: 2,871,443,918 sats to bcrt1prz4g6saush37epdwhvwpu78td3q7yfz3xxz37axlx7udck6wracq3rwq30 at 1/1 + # - 1 Output spend to another wallet: 319,049,328 sats to bcrt1p6p00wazu4nnqac29fvky6vhjnnhku5u2g9njss62rvy7e0yuperq86f5ek + # - Fee 155 sats + SINGLE_SIG_TAPROOT_WITH_CHANGE = "cHNidP8BAIkCAAAAAf8upuiIWF1VTgC/Q8ZWRrameRigaXpRcQcBe8ye+TK3AQAAAAAXCgAAAs7BJqsAAAAAIlEgGKqNQ7yF4+yFrrscHnjrbEHiJFExhR903ze43FtOH3BwTgQTAAAAACJRINBe93RcrOYO4UVLLE0y8pzvblOKQWcoQ0obCey8nA5GAAAAAE8BBDWHzwNMUx9OgAAAAJdr+WtwWfVa6IPbpKZ4KgRC0clbm11Gl155IPA27n2FAvQCrFGH6Ac2U0Gcy1IH5f5ltgUBDz2+fe8iqL6JzZdgEDlK7RRWAACAAQAAgAAAAIAAAQB9AgAAAAGAKOOUFIzw9pbRDaZ7F0DYhLImrdMn//OSm++ff5VNdAAAAAAAAQAAAAKsjLwAAAAAABYAFKEcuxvXmB3rWHSqSviP5mrKMZoL2RArvgAAAAAiUSBGU0Lg5fx/ECsB1Z4ZUqXQFSLFnlmpm0rm5R2l599h2AAAAAABASvZECu+AAAAACJRIEZTQuDl/H8QKwHVnhlSpdAVIsWeWambSublHaXn32HYAQMEAAAAACEWF7hZVn7pIDR429kAn/WDeQiWjZey1iGHztsL1H83QLMZADlK7RRWAACAAQAAgAAAAIABAAAAAAAAAAEXIBe4WVZ+6SA0eNvZAJ/1g3kIlo2XstYhh87bC9R/N0CzACEHbJdqWyMxF2eOPr6YRXUJmry04HUbgKyeM2IZeG+NI9AZADlK7RRWAACAAQAAgAAAAIABAAAAAQAAAAEFIGyXalsjMRdnjj6+mEV1CZq8tOB1G4CsnjNiGXhvjSPQAAA=" + SINGLE_SIG_INPUTS = [ SINGLE_SIG_NATIVE_SEGWIT_1_INPUT, SINGLE_SIG_NESTED_SEGWIT_1_INPUT, @@ -81,6 +117,13 @@ class PSBTTestData: MULTISIG_NESTED_SEGWIT_RECEIVE = "01030890d0030000000000010417a9141d3c080d4b05358b8cc439ef12534250563f34a08700" MULTISIG_LEGACY_P2SH_RECEIVE = "01030890d0030000000000010417a914fa70ebb69e283b493770c5a8fa19ec76da321de68700" + # The same output as MULTISIG_NATIVE_SEGWIT_RECEIVE, but populated the way a + # coordinator that also holds the recipient's descriptor populates it: the 2-of-3 + # script the output pays, plus a derivation path entry for each of its cosigners. + # Built from recipient_seed, recipient_multisig_key_2 and recipient_multisig_key_3, + # each at m/48h/1h/0h/2h/0/0. + MULTISIG_NATIVE_SEGWIT_RECEIVE_ANNOTATED = "0101695221027d99f92952795d306ef9b87a9d16c94bedc4b17613042a6f1b80af0e9c7839a32103208d94f8b4df19bcb76c309887904920064ccc417225520790426cb0e40fe6552103d36ac409c6198fee6eae72bf5c38424f3f2e2077a67aaf14d535fd1021b97c4953ae2202027d99f92952795d306ef9b87a9d16c94bedc4b17613042a6f1b80af0e9c7839a31c0f7d3df0300000800100008000000080020000800000000000000000220203208d94f8b4df19bcb76c309887904920064ccc417225520790426cb0e40fe6551ccd063d44300000800100008000000080020000800000000000000000220203d36ac409c6198fee6eae72bf5c38424f3f2e2077a67aaf14d535fd1021b97c491c2f54d8a930000080010000800000008002000080000000000000000001030890d003000000000001042200200936ff1943bbe5c037ad9e9839ca3effe24d8b22fd38cc2601c9007cc0f210a100" + ALL_EXTERNAL_OUTPUTS = [ SINGLE_SIG_NATIVE_SEGWIT_RECEIVE, SINGLE_SIG_NESTED_SEGWIT_RECEIVE, @@ -158,7 +201,37 @@ def foreign_public_key(derivation_path: str = "m/84h/1h/0h/0/0", seed: Seed = No return root_for_seed(seed).derive(derivation_path).get_public_key() -def claim_seed_owns_key(scope: InputScope | OutputScope, claimed_derivation_path: str, public_key: PublicKey, seed: Seed = None, is_taproot: bool = False): +def tapleaf_hash(public_key: PublicKey, leaf_version: int = 0xC0) -> bytes: + """ + The BIP-341 leaf hash of the simplest tapscript (a lone key checksig), built the same + way embit builds it when it signs a script path spend. + """ + OP_PUSHBYTES_32 = b"\x20" + OP_CHECKSIG = b"\xac" + leaf_script = script.Script(OP_PUSHBYTES_32 + public_key.xonly() + OP_CHECKSIG) + return tagged_hash("TapLeaf", bytes([leaf_version]) + leaf_script.serialize()) + + +# BIP-341's provably unspendable internal key (aka "NUMS" point: "Nothing Up My Sleeve"): +# the SHA256 of the standard uncompressed encoding of the secp256k1 generator point, +# lifted to a curve point. A taproot address built for script path spends only uses this +# as its internal key, so that nobody holds a key path to it. +NUMS_INTERNAL_KEY = PublicKey.from_xonly(unhexlify("50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0")) + + +def p2tr_with_script_tree(internal_public_key: PublicKey, merkle_root: bytes) -> script.Script: + """ + The scriptPubKey of a taproot output whose internal key is tweaked by a script tree's + merkle root, built the same way embit builds the empty-tree form. embit's script.p2tr + cannot build this one: it asks the script tree for its own tweak, and embit defines no + script tree type to ask. + """ + OP_1 = b"\x51" # segwit version byte + OP_PUSHBYTES_32 = b"\x20" + return script.Script(OP_1 + OP_PUSHBYTES_32 + internal_public_key.taproot_tweak(merkle_root).xonly()) + + +def claim_seed_owns_key(scope: InputScope | OutputScope, claimed_derivation_path: str, public_key: PublicKey, seed: Seed = None, is_taproot: bool = False, leaf_hashes: list = None): """ Writes a derivation into the scope claiming that seed owns public_key at claimed_derivation_path. @@ -167,6 +240,9 @@ def claim_seed_owns_key(scope: InputScope | OutputScope, claimed_derivation_path ties a fingerprint to the key written beside it. The fingerprint is all it takes, and that is published in every psbt the seed has ever been sent. So pass a public_key the seed does not own and the result is a psbt asserting ownership that does not exist. + + For taproot, leaf_hashes names the leaf scripts the key is used in. An entry naming + none of them is claiming to be the output's internal key; see tapleaf_hash. """ if seed is None: seed = PSBTTestData.seed @@ -175,6 +251,6 @@ def claim_seed_owns_key(scope: InputScope | OutputScope, claimed_derivation_path root_for_seed(seed).my_fingerprint, bip32.parse_path(claimed_derivation_path)) if is_taproot: - scope.taproot_bip32_derivations[public_key] = ([], derivation_path) + scope.taproot_bip32_derivations[public_key] = (leaf_hashes or [], derivation_path) else: scope.bip32_derivations[public_key] = derivation_path diff --git a/tests/screenshot_generator/generator.py b/tests/screenshot_generator/generator.py index 328bf700..5fa52a2c 100644 --- a/tests/screenshot_generator/generator.py +++ b/tests/screenshot_generator/generator.py @@ -449,10 +449,11 @@ def generate_screenshots(locale): ScreenshotConfig(psbt_views.PSBTOverviewView, screenshot_name="PSBTOverviewView_op_return", mock_context_manager=mock_psbt_with_op_return_loaded), ScreenshotConfig(psbt_views.PSBTOpReturnView, screenshot_name="PSBTOpReturnView_text", mock_context_manager=mock_psbt_with_op_return_loaded), ScreenshotConfig(psbt_views.PSBTOpReturnView, screenshot_name="PSBTOpReturnView_raw_hex_data", mock_context_manager=mock_psbt_with_op_return_raw_bytes_loaded), - ScreenshotConfig(psbt_views.PSBTAddressVerificationFailedView, dict(is_change=True, is_multisig=False), screenshot_name="PSBTAddressVerificationFailedView_singlesig_change"), - ScreenshotConfig(psbt_views.PSBTAddressVerificationFailedView, dict(is_change=False, is_multisig=False), screenshot_name="PSBTAddressVerificationFailedView_singlesig_selftransfer"), - ScreenshotConfig(psbt_views.PSBTAddressVerificationFailedView, dict(is_change=True, is_multisig=True), screenshot_name="PSBTAddressVerificationFailedView_multisig_change"), - ScreenshotConfig(psbt_views.PSBTAddressVerificationFailedView, dict(is_change=False, is_multisig=True), screenshot_name="PSBTAddressVerificationFailedView_multisig_selftransfer"), + ScreenshotConfig(psbt_views.PSBTSurplusDerivationPathsView), + ScreenshotConfig(psbt_views.PSBTMixedDerivationPathTypesView), + ScreenshotConfig(psbt_views.PSBTOutputOwnershipContradictionView), + ScreenshotConfig(psbt_views.PSBTAddressVerificationFailedView, dict(is_change=True), screenshot_name="PSBTAddressVerificationFailedView_multisig_change"), + ScreenshotConfig(psbt_views.PSBTAddressVerificationFailedView, dict(is_change=False), screenshot_name="PSBTAddressVerificationFailedView_multisig_selftransfer"), ScreenshotConfig(psbt_views.PSBTOutputOwnershipClaimFailedView), ScreenshotConfig(psbt_views.PSBTInputOwnershipClaimFailedView), ScreenshotConfig(psbt_views.PSBTSeedCannotSignView), diff --git a/tests/test_flows_psbt.py b/tests/test_flows_psbt.py index 18ba21ef..c21a3e05 100644 --- a/tests/test_flows_psbt.py +++ b/tests/test_flows_psbt.py @@ -1,10 +1,11 @@ from binascii import a2b_base64 -from embit.psbt import PSBT +from embit import bip32, script +from embit.psbt import PSBT, DerivationPath from base import FlowTest, FlowStep from psbt_testing_util import (PSBTTestData, claim_seed_owns_key, create_output, - foreign_public_key) + foreign_public_key, root_for_seed) from seedsigner.controller import Controller from seedsigner.views.view import MainMenuView @@ -24,24 +25,7 @@ class TestPSBTFlows(FlowTest): since the PSBT is not a self transfer it should enter the PSBTAddressDetailsView flow """ def load_psbt_into_decoder(view: scan_views.ScanView): - """ - PSBT Tx and Wallet Details - - Single Sig Wallet P2WPKH (Native Segwit) with no passphrase - - Regtest c751dc07 m/84'/1'/0' tpubDDZBrnxMxbVzqt8EoEiABPxeKzFWma5pra5UEbg3Wst1hrwr6feuvcy7Sov7cpuYx94ypuy1PQ9NDNoQagFs37wGALzLb5Ei3FvyJWPPPKZ - - 2 Inputs - - 56,522,834 sats - - 1,990,245,069 sats - - 4 Outputs - - 1 Output to another wallet (bcrt1q7cw0wzy8g6mq5qvkpvhnk5gsps5ncy3srp0n2j) of 123,456 sats - - 3 Outputs change - - 3 outputs to emulate a fake mix to increase privacy - - Change addresses are index 1/7, 1/8, 1/9 - - 1/7 address bcrt1q53j0xwuskuf5gnvynadh0hlazyy8srydlucrhg with amount 123,456 sats - - 1/8 address bcrt1q5gtw3zfp4cx67yk5q42q6j6rfza8aqcwpyyslv with amount 1,990,121,477 sats - - 1/9 address bcrt1q9rrg7399m43cn0yg4tz0v0ate89jgf2d6kpz7v with amount 56,399,242 sats - - Fee 272 sats - """ - view.decoder.add_data("cHNidP8BANgCAAAAAsTXZs3fz/dmGb6M80+jjvJZdYya+cw5bT/dGuhZFdSlAAAAAAD9////qo6xg/UZAvUkcbse1F+C9zbP/FeZNjThx7SCIn6eMCgBAAAAAP3///8EQOIBAAAAAAAWABSkZPM7kLcTRE2En1t33/0RCHgMjQXYnnYAAAAAFgAUKMaPRKXdY4m8iKrE9j+rycskJU1A4gEAAAAAABYAFPYc9wiHRrYKAZYLLztREAwpPBIwipVcAwAAAAAWABSiFuiJIa4NrxLUBVQNS0NIun6DDtoRAABPAQQ1h88DBcQGZIAAAAA+0J+jlNL3dpWwlnBi8Dx+Ipg4e6uvB3HdjzFPX7r9CAOOlAIxgII+/xCcj+XoEenKH7wj5s5wlu7Q7CCZWFLGLhA5Su0UVAAAgAEAAIAAAACAAAEA7QIAAAAEE6njX/fnvn7hbkKIRcxzNYFOSfbCdNeWnd7Fe/1UcQ0BAAAAAP3///8TqeNf9+e+fuFuQohFzHM1gU5J9sJ015ad3sV7/VRxDQMAAAAA/f///xOp41/3575+4W5CiEXMczWBTkn2wnTXlp3exXv9VHENBAAAAAD9////E6njX/fnvn7hbkKIRcxzNYFOSfbCdNeWnd7Fe/1UcQ0GAAAAAP3///8CUnheAwAAAAAWABRCfygPJ+Fjsx4BknYvvm3A3qKn2xJ/XQcAAAAAF6kU1I4TAst5nAj15ey7vwe5cM3OFq+HlhEAAAEBH1J4XgMAAAAAFgAUQn8oDyfhY7MeAZJ2L75twN6ip9sBAwQBAAAAIgYCo7sfm78RQY3B5n0ac/QF8VtMAzFnci+h5D1MtpgRY7oYOUrtFFQAAIABAACAAAAAgAEAAAAGAAAAAAEAcQIAAAABxY7wh0nsfJQfzWrD/9rN9BYsM+iOmPaO6I0ANFgO/PcAAAAAAP3///8CptiUAAAAAAAWABRIm4HhQY/TzOjeWSPRrbuJo9MlW826oHYAAAAAFgAU0z+0L2QSLGtyQTn8FhbCpcI7jbliAQAAAQEfzbqgdgAAAAAWABTTP7QvZBIsa3JBOfwWFsKlwjuNuQEDBAEAAAAiBgITHmebEANk81CraV4xZIpqkNjjw0tIvezl1Ism1NRH3Rg5Su0UVAAAgAEAAIAAAACAAQAAAAAAAAAAIgICuTT7WnuiUTpObjWnZFHzIeEvW9PTB+1LLVFNQJVFeIIYOUrtFFQAAIABAACAAAAAgAEAAAAHAAAAACICAk8f3hpc5C35chgSg+Pe2zZ9IhHREd4aKW2+yAMRIFeqGDlK7RRUAACAAQAAgAAAAIABAAAACQAAAAAAIgIDjt1CjvrnMMnjbmTNKUAYoKEDRbmKjNjbq+6Ppqj3bqQYOUrtFFQAAIABAACAAAAAgAEAAAAIAAAAAA==") + view.decoder.add_data(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_2_INPUTS) def load_seed_into_decoder(view: scan_views.ScanView): view.decoder.add_data("080115060387063104071857067618681125136207731354") @@ -193,9 +177,9 @@ class TestPSBTFlows(FlowTest): class TestPSBTOwnershipClaimRouting(FlowTest): """ - A psbt carrying an ownership claim that does not hold up is rejected while it is being - parsed, before the user is shown anything about the transaction. These cover the - routing that turns that rejection into a warning screen instead of a crash. + A psbt whose own description of itself does not hold up is refused during parsing, + before the user is shown anything about the transaction. These cover what the user + meets when that happens: a warning screen that ends the flow, not a crash. """ def _load_psbt_for_signing(self, psbt: PSBT, seed: Seed = None): @@ -250,6 +234,92 @@ class TestPSBTOwnershipClaimRouting(FlowTest): ]) + def test_surplus_derivation_paths_terminate_signing_flow(self): + """ + When an output names more derivation paths than its script can use, nothing in + the psbt says which key it actually pays, so parsing refuses it. + + The parser tests cover why that shape is refusable. This one covers what the user + gets: a warning that ends the flow, rather than a crash, and without first being + walked through the details of a transaction about to be discarded. + """ + other_root = root_for_seed(PSBTTestData.recipient_seed) + + psbt = PSBT.parse(a2b_base64(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_2_INPUTS)) + + # Output 2 pays a stranger and carries no derivation path entries of its own. + # Give it two, so nothing in the psbt says which key it pays. + for i in range(2): + derivation_path = bip32.parse_path(f"m/84h/1h/0h/0/{i}") + psbt.outputs[2].bip32_derivations[other_root.derive(derivation_path).get_public_key()] = \ + DerivationPath(other_root.my_fingerprint, derivation_path) + + self._load_psbt_for_signing(psbt, seed=PSBTTestData.two_input_seed) + + self.run_sequence([ + FlowStep(psbt_views.PSBTSelectSeedView, screen_return_value=0), + FlowStep(psbt_views.PSBTOverviewView, is_redirect=True), + FlowStep(psbt_views.PSBTSurplusDerivationPathsView, button_data_selection=psbt_views.PSBTSurplusDerivationPathsView.DISCARD), + FlowStep(MainMenuView), + ]) + + + def test_output_ownership_contradiction_terminates_signing_flow(self): + """ + When a psbt marks an output as paying us while its script pays a stranger, the + two cannot both be true, so parsing refuses it. + + The parser tests cover which shapes qualify. This one covers what the user gets: + a warning that ends the flow, before any transaction detail is rendered. + """ + psbt = PSBT.parse(a2b_base64(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_2_INPUTS)) + + # Output 0 is the wallet's own change. Repoint its script at a stranger but leave + # its derivation path entry in place, so the psbt still names a key this seed + # owns on an output that no longer pays it. Note that psbt.tx is rebuilt on every + # access, so the scriptPubKey has to be set on the output scope itself. + psbt.outputs[0].script_pubkey = script.p2wpkh(foreign_public_key()) + + self._load_psbt_for_signing(psbt, seed=PSBTTestData.two_input_seed) + + self.run_sequence([ + FlowStep(psbt_views.PSBTSelectSeedView, screen_return_value=0), + FlowStep(psbt_views.PSBTOverviewView, is_redirect=True), + FlowStep(psbt_views.PSBTOutputOwnershipContradictionView, button_data_selection=psbt_views.PSBTOutputOwnershipContradictionView.DISCARD), + FlowStep(MainMenuView), + ]) + + + def test_mixed_derivation_path_types_terminate_signing_flow(self): + """ + An input or output filling both derivation path maps at once is refused on that + shape alone. The check lives in the ownership scan, which runs over inputs as well + as outputs, so this one plants the shape on an input, the side the parser tests do + not cover. + + It ends the flow at its own warning before any transaction detail is rendered. + """ + other_root = root_for_seed(PSBTTestData.recipient_seed) + + psbt = PSBT.parse(a2b_base64(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_2_INPUTS)) + + # Input 0 already carries a segwit-v0 entry. Add a taproot one beside it, on a + # stranger's key so nothing here claims this seed: the refusal is on the shape + # alone, not on an ownership claim. + derivation_path = bip32.parse_path("m/86h/1h/0h/0/0") + psbt.inputs[0].taproot_bip32_derivations[foreign_public_key()] = ( + [], DerivationPath(other_root.my_fingerprint, derivation_path)) + + self._load_psbt_for_signing(psbt, seed=PSBTTestData.two_input_seed) + + self.run_sequence([ + FlowStep(psbt_views.PSBTSelectSeedView, screen_return_value=0), + FlowStep(psbt_views.PSBTOverviewView, is_redirect=True), + FlowStep(psbt_views.PSBTMixedDerivationPathTypesView, button_data_selection=psbt_views.PSBTMixedDerivationPathTypesView.DISCARD), + FlowStep(MainMenuView), + ]) + + def test_wrong_seed_routes_back_to_seed_selection_flow(self): """ The wrong seed for a psbt redirects before any transaction detail is rendered and diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index 0b770120..b1ec561a 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -7,16 +7,18 @@ from unittest.mock import patch from embit import bip32, script from embit.ec import PublicKey from embit.networks import NETWORKS -from embit.psbt import PSBT, DerivationPath +from embit.psbt import PSBT, DerivationPath, OutputScope from embit.descriptor import Descriptor from seedsigner.models.psbt_parser import (PSBTInputOwnershipClaimError, - PSBTOutputOwnershipClaimError, PSBTParser, PSBTSeedCannotSignError) + PSBTMixedDerivationPathTypesError, PSBTOutputOwnershipClaimError, + PSBTOutputOwnershipContradictionError, PSBTParser, PSBTSeedCannotSignError, + PSBTSurplusDerivationPathsError) from seedsigner.models.seed import Seed from seedsigner.models.settings_definition import SettingsConstants -from psbt_testing_util import (PSBTTestData, claim_seed_owns_key, create_output, - foreign_public_key, root_for_seed) +from psbt_testing_util import (NUMS_INTERNAL_KEY, PSBTTestData, claim_seed_owns_key, + create_output, foreign_public_key, p2tr_with_script_tree, root_for_seed, tapleaf_hash) @@ -338,43 +340,31 @@ class TestPSBTParser: assert psbt_parser.verify_multisig_output(descriptor, change_num=1) == False + def test__is_change_branch__distinguishes_the_change_branch_from_the_receive_branch(self): + """ + A wallet keeps its change addresses on branch 1 and the addresses it hands out to + other people on branch 0. The next-to-last element of the derivation path is + therefore what separates change from a self-transfer back to our own receive addr. + """ + assert PSBTParser.is_change_branch(bip32.parse_path("m/84h/1h/0h/1/0")) is True + assert PSBTParser.is_change_branch(bip32.parse_path("m/84h/1h/0h/0/0")) is False + + # TODO: Refactor all tests to be in the TestPSBTParser class(?) def test_p2tr_change_detection(): - """ Should successfully detect change in a p2tr to p2tr psbt spend - - PSBT Tx and Wallet Details - - Single Sig Wallet P2TR (Taproot) with no passphrase - - Regtest 394aed14 m/86'/1'/0' tpubDCawGrRg7YdHdFb9p4mmD8GBaZjJegL53FPFRrMkGoLcgLATJfksUs2y1Q7dVzixAkgecazsxEsUuyj3LyDw7eVVYHQyojwrc2hfesK4wXW - - 1 Inputs - - 3,190,493,401 sats - - 2 Outputs - - 1 Output spend to another wallet (bcrt1p6p00wazu4nnqac29fvky6vhjnnhku5u2g9njss62rvy7e0yuperq86f5ek) p2tr address - - 1 Output change - - addresss bcrt1prz4g6saush37epdwhvwpu78td3q7yfz3xxz37axlx7udck6wracq3rwq30) - - amount 2,871,443,918 sats - - Change addresses is index 1/1 - - Fee 155 sats - """ - - psbt_base64 = "cHNidP8BAIkCAAAAAf8upuiIWF1VTgC/Q8ZWRrameRigaXpRcQcBe8ye+TK3AQAAAAAXCgAAAs7BJqsAAAAAIlEgGKqNQ7yF4+yFrrscHnjrbEHiJFExhR903ze43FtOH3BwTgQTAAAAACJRINBe93RcrOYO4UVLLE0y8pzvblOKQWcoQ0obCey8nA5GAAAAAE8BBDWHzwNMUx9OgAAAAJdr+WtwWfVa6IPbpKZ4KgRC0clbm11Gl155IPA27n2FAvQCrFGH6Ac2U0Gcy1IH5f5ltgUBDz2+fe8iqL6JzZdgEDlK7RRWAACAAQAAgAAAAIAAAQB9AgAAAAGAKOOUFIzw9pbRDaZ7F0DYhLImrdMn//OSm++ff5VNdAAAAAAAAQAAAAKsjLwAAAAAABYAFKEcuxvXmB3rWHSqSviP5mrKMZoL2RArvgAAAAAiUSBGU0Lg5fx/ECsB1Z4ZUqXQFSLFnlmpm0rm5R2l599h2AAAAAABASvZECu+AAAAACJRIEZTQuDl/H8QKwHVnhlSpdAVIsWeWambSublHaXn32HYAQMEAAAAACEWF7hZVn7pIDR429kAn/WDeQiWjZey1iGHztsL1H83QLMZADlK7RRWAACAAQAAgAAAAIABAAAAAAAAAAEXIBe4WVZ+6SA0eNvZAJ/1g3kIlo2XstYhh87bC9R/N0CzACEHbJdqWyMxF2eOPr6YRXUJmry04HUbgKyeM2IZeG+NI9AZADlK7RRWAACAAQAAgAAAAIABAAAAAQAAAAEFIGyXalsjMRdnjj6+mEV1CZq8tOB1G4CsnjNiGXhvjSPQAAA=" - - raw = a2b_base64(psbt_base64) + """ Should successfully detect change in a p2tr to p2tr psbt spend """ + raw = a2b_base64(PSBTTestData.SINGLE_SIG_TAPROOT_WITH_CHANGE) tx = PSBT.parse(raw) - - mnemonic = "goddess rough corn exclude cream trial fee trumpet million prevent gaze power".split() - pw = "" - seed = Seed(mnemonic, passphrase=pw) - pp = PSBTParser(p=tx, seed=seed, network=SettingsConstants.REGTEST) + pp = PSBTParser(p=tx, seed=PSBTTestData.two_input_seed, network=SettingsConstants.REGTEST) assert pp.change_data == [ { 'output_index': 0, 'address': 'bcrt1prz4g6saush37epdwhvwpu78td3q7yfz3xxz37axlx7udck6wracq3rwq30', 'amount': 2871443918, - 'claimed_fingerprints': ['394aed14'], - 'claimed_derivation_paths': ['m/86h/1h/0h/1/1']} + 'verified_derivation_path': bip32.parse_path('m/86h/1h/0h/1/1')} ] assert pp.spend_amount == 319049328 assert pp.change_amount == 2871443918 @@ -533,8 +523,7 @@ def test_parse_op_return_content(): 'output_index': 0, 'address': 'bcrt1qvwkhakqhz7m7kmz6332avatsmdy32m644g86vv', 'amount': 99992296, - 'claimed_fingerprints': ['0fb882ff'], - 'claimed_derivation_paths': ["m/84h/1h/0h/0/2"]} + 'verified_derivation_path': bip32.parse_path("m/84h/1h/0h/0/2")} ] assert psbt_parser.spend_amount == 0 # This is a self-spend; no value being spent, other than the tx fee assert psbt_parser.change_amount == 99992296 @@ -825,10 +814,12 @@ class TestPSBTParserOptimizations: -class TestPSBTParserSeedOwnership: +class PSBTParserOwnershipTestBase: """ - The ownership scan: what the signing seed provably owns in a psbt, and the rejection - of any psbt whose ownership claims do not hold up. + Base class for the two ownership test classes below. Provides: + * the signing seed + * a psbt made of nothing but that seed's own scopes + * the parse call under test. """ seed = PSBTTestData.seed @@ -855,6 +846,12 @@ class TestPSBTParserSeedOwnership: return PSBTParser(psbt, self.seed, network=SettingsConstants.REGTEST) + +class TestPSBTParserSeedOwnership(PSBTParserOwnershipTestBase): + """ + The ownership scan: what the signing seed provably owns in a psbt and the rejection + of any psbt whose ownership claims do not hold up. + """ def test__seed_owns_pubkey__accepts_the_seeds_own_key(self): """ seed_owns_pubkey should confirm the simple base case that a pubkey directly @@ -1238,3 +1235,656 @@ class TestPSBTParserSeedOwnership: with pytest.raises(PSBTSeedCannotSignError): self._parse(psbt) + + + +class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): + """ + A psbt annotates its outputs with claims about which keys own them by providing + derivation path entries for each key. + + But it's the output's script that actually determines where the funds go. + + These tests cover invalid claims as well as the ways that the claims and the script + can disagree and how PSBTParser handles such discrepancies. + """ + def _foreign_multisig_script(self) -> script.Script: + """A 2-of-3 built entirely from someone else's keys.""" + return script.multisig(2, [foreign_public_key(f"m/48h/1h/0h/2h/0/{i}") for i in range(3)]) + + + def _rebuild_around_foreign_keys(self, out: OutputScope): + """ + Replace a multisig output's script to pay to a different quorum made entirely of + someone else's keys, but leave the output's original derivation path entries + alone. The output now misrepresents who receives its funds. + """ + foreign_script = self._foreign_multisig_script() + + if out.witness_script is not None: + out.witness_script = foreign_script + inner_script = script.p2wsh(foreign_script) + else: + inner_script = foreign_script + + if out.redeem_script is not None: + out.redeem_script = inner_script + out.script_pubkey = script.p2sh(inner_script) + else: + out.script_pubkey = inner_script + + + def test__parse__rejects_a_single_key_output_that_claims_more_than_one_path(self): + """ + It is nonsensical for a single sig output to list more than one derivation path + entry. Some elaborate deceptions may be possible with extra derivation paths, but + we simply reject such psbts by raising PSBTSurplusDerivationPathsError. + """ + decoy_derivation_path = "m/84h/1h/0h/1/9" + + # A normal psbt with change; includes the single derivation path entry that + # describes it. + psbt = self._psbt_with_change() + + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 10_000 + + # The same psbt but with a second entry naming a key this seed really does own at + # another path. + psbt = self._psbt_with_change() + decoy_public_key = self._root().derive(decoy_derivation_path).get_public_key() + claim_seed_owns_key(psbt.outputs[0], decoy_derivation_path, decoy_public_key) + + with pytest.raises(PSBTSurplusDerivationPathsError): + self._parse(psbt) + + + def test__parse__rejects_a_taproot_output_that_claims_more_than_one_internal_key(self): + """ + Taproot is an exception to the rule that single sig outputs cannot have more than + one derivation path entry. A BIP-371 output can legitimately have several entries: + the internal key plus one for each key in its script tree. + + But the basic single sig logic holds for the internal key: there can be only one. + + Entries that do name a leaf are legitimate and should be left out of the count. + """ + decoy_derivation_path = "m/86h/1h/0h/1/7" + decoy_public_key = self._root().derive(decoy_derivation_path).get_public_key() + + leaf_derivation_path = "m/86h/1h/0h/1/8" + leaf_public_key = self._root().derive(leaf_derivation_path).get_public_key() + + # The wallet's own taproot change, carrying the single entry that describes it + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + + # The simple case parses and identifies the output as change + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 10_000 + + # The same psbt, with a second entry naming no leaf hashes, which is therefore a + # second claim to be the output's one internal key. + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + claim_seed_owns_key(psbt.outputs[0], decoy_derivation_path, decoy_public_key, is_taproot=True) + + with pytest.raises(PSBTSurplusDerivationPathsError): + self._parse(psbt) + + # The same two internal key entries, now with a genuine script tree key beside + # them. The surplus is still a surplus. + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + claim_seed_owns_key(psbt.outputs[0], decoy_derivation_path, decoy_public_key, is_taproot=True) + claim_seed_owns_key(psbt.outputs[0], leaf_derivation_path, leaf_public_key, is_taproot=True, + leaf_hashes=[tapleaf_hash(leaf_public_key)]) + + with pytest.raises(PSBTSurplusDerivationPathsError): + self._parse(psbt) + + + def test__parse__rejects_an_output_with_entries_in_both_derivation_path_maps(self): + """ + The taproot derivation paths (taproot_bip32_derivations) are only relevant for + taproot scripts, just as the non-taproot derivation paths (bip32_derivations) are + only relevant for non-taproot scripts. + + There is no scenario where both maps could carry valid data for the same output. + + A psbt that includes both is rejected with PSBTMixedDerivationPathTypesError. + """ + # The wallet's own taproot change, with an empty segwit-v0 map beside it + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + assert len(psbt.outputs[0].bip32_derivations) == 0 + assert len(psbt.outputs[0].taproot_bip32_derivations) == 1 + + # Parses successfully as expected + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 10_000 + + # The same psbt, with a truthful entry on a key this seed owns written into the + # non-taproot map. + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + + extra_derivation_path = "m/84h/1h/0h/1/9" + extra_public_key = self._root().derive(extra_derivation_path).get_public_key() + claim_seed_owns_key(psbt.outputs[0], extra_derivation_path, extra_public_key) + + # The output now has entries in both maps + assert len(psbt.outputs[0].bip32_derivations) == 1 + assert len(psbt.outputs[0].taproot_bip32_derivations) == 1 + + # So it fails as expected + with pytest.raises(PSBTMixedDerivationPathTypesError): + self._parse(psbt) + + # Set up the same collision, but with a non-taproot output + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_CHANGE) + assert len(psbt.outputs[0].bip32_derivations) == 1 + assert len(psbt.outputs[0].taproot_bip32_derivations) == 0 + + # Add a valid taproot derivation path entry + taproot_derivation_path = "m/86h/1h/0h/1/9" + taproot_public_key = self._root().derive(taproot_derivation_path).get_public_key() + claim_seed_owns_key(psbt.outputs[0], taproot_derivation_path, taproot_public_key, is_taproot=True) + + # The output now has entries in both maps + assert len(psbt.outputs[0].bip32_derivations) == 1 + assert len(psbt.outputs[0].taproot_bip32_derivations) == 1 + + # Once again fails as expected + with pytest.raises(PSBTMixedDerivationPathTypesError): + self._parse(psbt) + + + def test__parse__counts_a_multisig_output_paying_other_people_as_a_spend(self): + """ + Most coordinators will not provide any output derivation paths nor the output + script itself for external spends. In most cases the coordinator simply wouldn't + know that information for outside parties. + + But even if that information is provided (as Bitcoin Core can do if the recipient + is another wallet for which it knows the internal details), the parser should + still interpret the output correctly: as an external spend. + + Most of the tests in this class are about what is not allowed. This test gives the + parser a scenario that IS allowed, but it touches on many of the areas that the + parser's rejection logic depends on. + """ + psbt = PSBT.parse(a2b_base64(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT)) + recipient_output = create_output(PSBTTestData.MULTISIG_NATIVE_SEGWIT_RECEIVE_ANNOTATED, 10_000) + psbt.outputs.append(recipient_output) + + # The coordinator's annotation is honest: the script it supplied is the one the + # output really pays, and the keys it names are the recipient's own. + assert script.p2wsh(recipient_output.witness_script).data == recipient_output.script_pubkey.data + assert len(recipient_output.bip32_derivations) == 3 + + # As expected: no errors raised, no deceptions detected + psbt_parser = self._parse(psbt) + + # Trivial confirmation: none of the output's three derivation path entries claimed + # to belong to this seed. + assert psbt_parser.verified_output_derivation_paths[0] is None + + # The parser correctly categorized the output as an external spend + assert psbt_parser.change_data == [] + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + # BIP-174 makes the derivation path entries optional, so a coordinator may supply + # the output's script and nothing else. Only the entries are dropped here; the + # output still commits to the same outside parties' script. + recipient_output.bip32_derivations.clear() + + # Still parses successfully; no errors raised, no deceptions detected + psbt_parser = self._parse(psbt) + + # Same result. The output was correctly categorized as an external spend. + assert psbt_parser.change_data == [] + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + + def test__parse__counts_multisig_change_with_no_derivation_paths_as_a_spend(self): + """ + The same shape as an outgoing payment, but this output really is our own change: + the script it commits to does hold a key of this seed. BIP-174 makes the entries + naming that key optional, and this psbt omits them. + + With no entry to derive from there is no key to go looking for in the script, so + the output is counted as a spend. That over-reports what is leaving the wallet, + and it is a limit on what we can see rather than a detection of anything wrong. + """ + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE) + + # Unmodified, this output is the wallet's own change + psbt_parser = self._parse(psbt) + + # With the derivation paths present, we verified that the output did name a key + # that this seed owns (which also enabled the parser to verify that our key was + # indeed part of the script). + assert psbt_parser.verified_output_derivation_paths[0] is not None + + # And the output was correctly categorized as change + assert psbt_parser.change_amount == 10_000 + assert psbt_parser.spend_amount == 0 + + # Dropping the derivation path entries leaves the coordinator's witness_script + # intact, so the output still hashes to the committed scriptPubKey and still + # matches the input's policy. Only the path we would derive our key from is gone. + psbt.outputs[0].bip32_derivations = {} + + psbt_parser = self._parse(psbt) + + # The output provided no derivation paths to verify (leaving the parser unable to + # determine if our seed owns any of the keys in the output's script). + assert psbt_parser.verified_output_derivation_paths[0] is None + + # Because we couldn't do proper verification, the parser correctly categorized the + # output as an external spend. + assert psbt_parser.change_data == [] + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + + def test__parse__counts_multisig_change_with_no_script_as_a_spend(self): + """ + The same output as the test above, withholding the other half. Here the entries + naming this seed are intact and the supplied script is what is missing, which + BIP-174 also permits. + + So the psbt claims the output for this seed and gives us nothing to check that + claim against. A claim we cannot check is not a contradiction, so the psbt is + not rejected, but the output is counted as a spend. + """ + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE) + + # The entries claiming this seed are left alone; only the script is withheld + psbt.outputs[0].witness_script = None + + psbt_parser = self._parse(psbt) + + # The claim itself still verifies + assert psbt_parser.verified_output_derivation_paths[0] is not None + + # But with no script there is no m-of-n to compare, so the output never becomes a + # change candidate at all. + assert psbt_parser.change_data == [] + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + + def test__parse__counts_taproot_change_naming_our_internal_key_as_a_spend(self): + """ + SeedSigner would not be used (yet) for a taproot wallet that includes a script + tree, but the parser already has to distinguish a derivation path for the internal + key vs derivation paths for tapleaf keys. So this test and its sibling that + follows verify that the parser correctly handles tapleaf keys when encountered + (within the limitations we have due to not parsing the script tree itself). + + This test provides a derivation path for the internal key, which our seed owns. + The wallet can spend the output through the key path, so the funds really are this + seed's own change. But the address commits to that internal key tweaked by the + script tree which we do not yet parse, so the parser's attempts at validating the + output's script will fail. + + As a result, we have to treat this output as an external spend. An output cannot + be categorized as change if we have not fully verified it. + """ + # TODO: When the script tree is supported, this output should verify as change, by + # tweaking our internal key with the tree's merkle root. + + # A taproot address where we control both spending routes: the internal key path + # and a one-leaf script tree. The coordinator annotates both keys in the psbt so + # the parser will see the derivation path for the internal key and the tapleaf + # key. + leaf_derivation_path = "m/86h/1h/0h/1/7" + leaf_public_key = self._root().derive(leaf_derivation_path).get_public_key() + merkle_root = tapleaf_hash(leaf_public_key) + + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + taproot_output = psbt.outputs[0] + + # The change output's key has already been annotated as the internal key. We just + # need to add the tapleaf key's entry. + taproot_output.script_pubkey = p2tr_with_script_tree(taproot_output.taproot_internal_key, merkle_root) + claim_seed_owns_key(taproot_output, leaf_derivation_path, leaf_public_key, is_taproot=True, leaf_hashes=[merkle_root]) + + psbt_parser = self._parse(psbt) + + # Even though the parser verified that our seed owns the internal key... + assert psbt_parser.verified_output_derivation_paths[0] is not None + + # ...the parser can't fully verify the output as change, so has to report it as an + # external spend. + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + + def test__parse__counts_taproot_change_naming_our_script_tree_key_as_a_spend(self): + """ + Sibling to the above test. This time no derivation path is provided for the + internal key (BIP-174 says that every entry is optional) and the sole derivation + path entry names a tapleaf key instead. + + That is what a real-world script-path-only address looks like: intentionally + constructed so that nobody holds the internal key, so there's no derivation path + the coordinator could supply for it. Our seed owns the key in the leaf and can + spend the output through it, so this too is the wallet's own change. But since + embit doesn't yet parse the taproot script tree, the parser cannot verify the + output's script. So the output must be reported as a spend. + """ + # TODO: When the script tree is supported this output should verify as change, by + # finding our key among the tree's leaves. + + # BIP 341's provably unspendable internal key (the "NUMS" point), over a one-leaf + # script tree holding a key this seed does own. With no internal key to describe, + # the one entry the coordinator writes is for the leaf key. + leaf_derivation_path = "m/86h/1h/0h/1/7" + leaf_public_key = self._root().derive(leaf_derivation_path).get_public_key() + merkle_root = tapleaf_hash(leaf_public_key) + + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + taproot_output = psbt.outputs[0] + taproot_output.script_pubkey = p2tr_with_script_tree(NUMS_INTERNAL_KEY, merkle_root) + + # Ensure that there's no derivation path entry for the internal key + taproot_output.taproot_bip32_derivations.clear() + + # Add the one and only derivation path claim: the tapleaf key + claim_seed_owns_key(taproot_output, leaf_derivation_path, leaf_public_key, is_taproot=True, leaf_hashes=[merkle_root]) + + psbt_parser = self._parse(psbt) + + # The parser verified that we own the tapleaf key... + assert psbt_parser.verified_output_derivation_paths[0] is not None + + # ...but the output still has to be reported as an external spend + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + + def test__parse__rejects_an_output_that_claims_this_seed_but_pays_someone_else(self): + """ + The output's derivation path entry claims that the output pays to a key that our + seed genuinely owns, but the output's script actually pays out to a different key. + We consider this deception an attack. + """ + # First the normal case where it really is our change + psbt = self._psbt_with_change() + psbt_parser = self._parse(psbt) + + # Confirmed by the parser + assert psbt_parser.change_amount == 10_000 + + # The same psbt, but with that output's script repointed at a stranger while its + # original derivation path entry is left in place. The output now misrepresents + # who receives its funds. + psbt = self._psbt_with_change() + psbt.outputs[0].script_pubkey = script.p2wpkh(foreign_public_key()) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + # Removing the derivation path entry means that there is now no longer any + # deception; the output is just an external spend. + psbt = self._psbt_with_change() + psbt.outputs[0].script_pubkey = script.p2wpkh(foreign_public_key()) + psbt.outputs[0].bip32_derivations.clear() + + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + + def test__parse__rejects_an_output_that_pays_this_seed_but_claims_someone_else(self): + """ + This is the opposite deception from the previous test. A psbt might mark our own + change with a derivation path that claims it belongs to someone else (provides + someone else's fingerprint). + + We don't try to decide whether this is an attack or a mistake. Any incorrect + claim about ownership is a deception, so the psbt is rejected. + """ + # First the normal case where it really is our change + psbt = self._psbt_with_change() + psbt_parser = self._parse(psbt) + + # Confirmed by the parser + assert psbt_parser.change_amount == 10_000 + + # The same psbt, with the output now claiming a stranger's fingerprint. The key + # that the output's script actually pays out to is still our key and the derivation + # path is untouched. + psbt = self._psbt_with_change() + public_key, derivation_path_obj = list(psbt.outputs[0].bip32_derivations.items())[0] + + # Create the deception: Change the fingerprint, but keep the derivation path + psbt.outputs[0].bip32_derivations[public_key] = DerivationPath( + root_for_seed(PSBTTestData.recipient_seed).my_fingerprint, derivation_path_obj.derivation) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + + def test__parse__rejects_taproot_change_that_claims_someone_else(self): + """ + The taproot mirror of the test above. The output's scriptPubKey still pays our + internal key at the derivation path the psbt supplies, but the entry claims a + stranger's fingerprint, so nothing verifies as ours and the psbt is refused. + + Worth its own test because the p2tr branch reaches that refusal by a different + route, taking the path from the single internal-key entry rather than from + bip32_derivations. + """ + # First the normal case where it really is our change + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + psbt_parser = self._parse(psbt) + + assert psbt_parser.change_amount == 10_000 + + # The same psbt, with only the fingerprint on the internal key's entry replaced. + # The scriptPubKey and the derivation path are left alone, so the output still + # pays the key that path produces. + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_TAPROOT_1_INPUT, PSBTTestData.SINGLE_SIG_TAPROOT_CHANGE) + public_key, (leaf_hashes, derivation_path_obj) = list(psbt.outputs[0].taproot_bip32_derivations.items())[0] + + # The entry has to stay an internal-key claim, so its (empty) leaf hashes are kept + assert leaf_hashes == [] + psbt.outputs[0].taproot_bip32_derivations[public_key] = ( + leaf_hashes, + DerivationPath(root_for_seed(PSBTTestData.recipient_seed).my_fingerprint, derivation_path_obj.derivation)) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + + def test__parse__rejects_a_multisig_output_built_from_other_peoples_keys(self): + """ + A malicious psbt might try to trick the parser with a multisig output that claims + to be the wallet's change, but whose script is built entirely from someone else's + keys. + + The output's script type and m-of-n match the inputs', so it is considered as a + possible change output. The attacker must also annotate the output with a + derivation path entry that names a key our seed really owns in order to pass other + parser checks. This creates a false claim that the change output belongs to our + seed. + + But a final check prevents this deception from succeeding: + + An output should only be considered change when one of our keys is in the output's + script. + + Raise PSBTOutputOwnershipContradictionError if this deception is detected. + """ + # For each multisig script type... + for input_base64, change_hex in [ + (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE), + ]: + # First create the psbt with the wallet's correct change + psbt = self._psbt_with_change(input_base64, change_hex) + + # The parse is still successful and raises no alarms + psbt_parser = self._parse(psbt) + + assert psbt_parser.change_amount == 10_000 + assert psbt_parser.spend_amount == 0 + + # The same psbt but now make the output pay to someone else's keys + psbt = self._psbt_with_change(input_base64, change_hex) + self._rebuild_around_foreign_keys(psbt.outputs[0]) + + # The output now still claims to pay our seed but the actual output script + # says otherwise. The deception is flagged. + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + + def test__parse__rejects_a_multisig_output_that_hides_this_seed_behind_another_fingerprint(self): + """ + This scenario leaves a genuine multisig change output completely intact, but + relabels the entry describing this seed's key with a different fingerprint. + + The seed's key is still in the script the output commits to, and the psbt still + supplies the derivation path that produced our key, so deriving at that path + proves this is actually our change. + + But the contradicting fingerprint claimed in the derivation path entry is a + deception that we consider an attack. Raises + PSBTOutputOwnershipContradictionError. + """ + for input_base64, change_hex in [ + (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE), + ]: + # Untouched, the output is recognized as change coming back to the seed + psbt_parser = self._parse(self._psbt_with_change(input_base64, change_hex)) + assert psbt_parser.change_amount == 10_000 + + # The same psbt with only the fingerprint on this seed's entry replaced. The + # script, the scriptPubKey and every derivation path are left alone. + psbt = self._psbt_with_change(input_base64, change_hex) + root = self._root() + entries = psbt.outputs[0].bip32_derivations + + relabeled = 0 + for public_key, derivation_path_obj in list(entries.items()): + if derivation_path_obj.fingerprint == root.my_fingerprint: + entries[public_key] = DerivationPath(b"\x11\x22\x33\x44", derivation_path_obj.derivation) + relabeled += 1 + + # The tamper only means anything if it actually landed on this seed's entry + assert relabeled == 1 + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + + def test__parse__refuses_a_multisig_decoy_entry_in_either_position(self): + """ + In this scenario the multisig change output is a legitimate change output that + genuinely belongs to our seed, but a decoy derivation path entry is added. The + decoy is ALSO a key that our seed owns, but it is not used in the output's script. + + We don't need to decide if such a psbt has malicious intent; the fact that it + contradicts itself is unacceptable regardless: + * it names a key on an output whose script does not use it. + * it names more keys than that script has. + Both are provable from the psbt alone, so we reject the psbt. + + This is similar to the single sig test earlier in this class, but is more + complicated for multisig since it's the norm for multiple derivation paths to be + provided for each multisig change output. + + The derivation path entries are provided in a coordinator-controlled order, so + this test covers decoy entries that are listed before or after the seed's actual + cosigner entry, across all three multisig script types. + + Both orderings are refused. The ordering only decides which problem we report. + We record the first entry that verifies against our seed, so: + * When the decoy is listed first, the decoy is what we record and it is not in + the script. + * When the decoy is listed last, the key we record is our real one and nothing + is wrong with it; what gives the decoy away instead is that the output named + more keys than its script has. + """ + root = self._root() + + # For each script type... + for input_base64, change_hex in [ + (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE), + ]: + # ...run both versions of the test: decoy listed first and decoy last + for decoy_first in [True, False]: + psbt = self._psbt_with_change(input_base64, change_hex) + + cosigner_entries = dict(psbt.outputs[0].bip32_derivations) + + # Build the decoy from the cosigners' baseline, then make one minor + # derivation path change. + genuine_derivation_path = list(cosigner_entries.values())[0].derivation + decoy_derivation_path = genuine_derivation_path[:-1] + [genuine_derivation_path[-1] + 1] + decoy_public_key = root.derive(decoy_derivation_path).get_public_key() + decoy_entry = DerivationPath(root.my_fingerprint, decoy_derivation_path) + + # Add the decoy to the existing 3 derivations + entries = psbt.outputs[0].bip32_derivations + if decoy_first: + entries.clear() + entries[decoy_public_key] = decoy_entry + entries.update(cosigner_entries) + else: + entries[decoy_public_key] = decoy_entry + + if decoy_first: + # The parser uses the decoy as the comparison against which keys are + # actually in the script. + expected_error = PSBTOutputOwnershipContradictionError + else: + # The original cosigner is verified but then the parser detects the + # decoy as a surplus derivation path. + expected_error = PSBTSurplusDerivationPathsError + + with pytest.raises(expected_error): + self._parse(psbt) + + + def test__parse__rejects_a_multisig_output_whose_supplied_script_is_not_its_own(self): + """ + This is a change-theft scenario where a malicious coordinator replaces just the + scriptPubKey on a multisig change output. As a result, every check on the change + output correctly passes, right up until the output's script is hashed and compared + to the output's scriptPubKey. The two results are different, proving that the + output's funds are not going where the psbt claimed they were. + """ + for input_base64, change_hex in [ + (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE), + ]: + # First create the psbt with the wallet's correct change + psbt = self._psbt_with_change(input_base64, change_hex) + psbt_parser = self._parse(psbt) + + # Categorizes the output correctly as change coming back to the seed + assert psbt_parser.change_amount == 10_000 + + # The same psbt, with only the scriptPubKey repointed at a stranger's + # 2-of-3. The psbt's own witness/redeem script and the entries naming this + # seed are left untouched, so the psbt still holds its original claims + # about who owns the output. + psbt = self._psbt_with_change(input_base64, change_hex) + out = psbt.outputs[0] + + foreign_script = self._foreign_multisig_script() + inner_script = script.p2wsh(foreign_script) if out.witness_script is not None else foreign_script + out.script_pubkey = script.p2sh(inner_script) if out.redeem_script is not None else inner_script + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + From 12c723917b967caf64fb1dd4dc663be95ec0ab15 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:45:25 -0500 Subject: [PATCH 2/6] Set is_change only after each branch's own checks In _parse_outputs the flag was set True as soon as the rebuilt scriptPubKey matched, and two multisig branches then turned it back off. That made "change" the default inside the matched arm, so a branch that forgot the flag would record change with nothing verified behind it. Now each branch sets the flag only once its own checks have passed, and an output that satisfies none of them stays an external spend. The two sites where the flag is set split by signature type, and each carries the certainty it actually has: single sig is settled, because our own key rebuilt the scriptPubKey; multisig is a presumption, because our key in the committed script does not say which wallet that script belongs to, and only the user's descriptor can. The reminder that "change" means any output coming back to our seed moves to the line where change_data is built, which is where the word takes that meaning. No behavior change: the same outputs land in the same lists on every fixture shape, and the suite is unchanged. --- src/seedsigner/models/psbt_parser.py | 30 +++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index d4fd01d2..75d886c2 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -478,9 +478,6 @@ class PSBTParser(): # The scriptPubKey we created using our own seed matched what this # output is actually committing to. - # Remember that "change" is ANY output coming back to our seed - is_change = True - if singlesig_derivation_path is not None: if verified_derivation_path is None: # The output pays this seed but the psbt claimed a different @@ -494,6 +491,11 @@ class PSBTParser(): # only have verified this same path. raise RuntimeError(f"Output {i} verified at a path it does not pay") + # We've now verified that the key we derived from our seed at the + # claimed path is the key this output pays. This output is + # provably ours. + is_change = True + elif multisig_script is not None: if verified_derivation_path is None: # No entry claimed this seed's fingerprint, but we already @@ -512,10 +514,11 @@ class PSBTParser(): # We treat this deception as an attack. raise PSBTOutputOwnershipContradictionError(f"Output's committed script holds this seed's key at {bip32.path_to_str(derivation_path_obj.derivation)} but the psbt claims another fingerprint and/or public key there") - # Every path the psbt supplied has been checked and none puts - # this seed in the committed script, so the output is an - # external spend. - is_change = False + # We have derived a key from our seed for every derivation + # path this output supplies, but none of our keys match any + # of the keys in this output's script. So we consider this + # output an external spend. + pass else: # This output claimed that our seed is part of the receiving @@ -536,6 +539,15 @@ class PSBTParser(): # mistake. We just abort the parse. raise PSBTSurplusDerivationPathsError("Multisig output claims more derivation paths than its script has keys") + # We now know that our key is in the committed script; this + # output does pay to a multisig that our seed is part of. But + # note that we do not know yet if this is truly change coming + # back to our wallet or if it is paying out to a different + # multisig that happens to include our seed. Final change + # verification can only happen if and when the user loads + # their "known-good" multisig descriptor. + is_change = True + else: # No handler claimed a matching output, which the branches above # should make impossible. Raise rather than leave is_change True; @@ -569,6 +581,10 @@ class PSBTParser(): self.op_return_data = vout[i].script_pubkey.data[3:] elif is_change: + # Remember that "change" in this function is ANY output coming back to our + # seed, receive addresses included. It is up to the View layer to use the + # derivation path to determine if it should be displayed as change or + # receive. addr = vout[i].script_pubkey.address(NETWORKS[SettingsConstants.map_network_to_embit(self.network)]) self.change_data.append({ "output_index": i, From a929b8b35bde554f434b7b199580dfde2565b2c1 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:06:12 -0500 Subject: [PATCH 3/6] Match policy types exactly and refuse an unsupported one The rebuild chain in _parse_outputs matched single sig and taproot by substring ("pkh" in type, "p2tr" in type) and had no else. embit reports a script type it does not recognize as None, and _get_policy passes that through, so a psbt whose inputs and outputs were both unrecognized passed the shape gate and died in the substring test with a bare TypeError. The guard meant for an unhandled type sat inside the scriptPubKey match, where an empty rebuild cannot arrive. The arms now name their types exactly, matching _build_singlesig_script, and the chain ends in a catch-all else that raises RuntimeError with the unsupported type in the message. The unreachable inner guard is removed. A test drives a p2pk psbt into the catch-all. --- src/seedsigner/models/psbt_parser.py | 21 ++++++++++----------- tests/test_psbt_parser.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 75d886c2..f0eaf61d 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -421,9 +421,8 @@ class PSBTParser(): multisig_script = out.redeem_script rebuilt_script_pubkey = script.p2sh(multisig_script) - # single-sig: p2pkh, p2sh-p2wpkh, and p2wpkh; taproot handled separately - # below. - elif "pkh" in self.policy["type"]: + # single-sig; taproot handled separately below. + elif self.policy["type"] in ("p2pkh", "p2sh-p2wpkh", "p2wpkh"): # Sanity check; a single sig output shouldn't have multiple derivation # paths. if len(out.bip32_derivations) > 1: @@ -439,7 +438,7 @@ class PSBTParser(): # considered an external spend. pass - elif "p2tr" in self.policy["type"]: + elif self.policy["type"] == "p2tr": taproot_entries = list(out.taproot_bip32_derivations.values()) if len(taproot_entries) == 0: @@ -472,6 +471,12 @@ class PSBTParser(): # TODO: Support keys in script tree leaves pass + else: + # Safety catch-all: any new script types will need explicit handling + # above. Note that embit reports unrecognized script types as `None`, + # which is also caught here. + raise RuntimeError(f"Unsupported policy type: {self.policy['type']}") + verified_derivation_path = self.verified_output_derivation_paths[i] if rebuilt_script_pubkey.data == vout[i].script_pubkey.data: @@ -548,13 +553,7 @@ class PSBTParser(): # their "known-good" multisig descriptor. is_change = True - else: - # No handler claimed a matching output, which the branches above - # should make impossible. Raise rather than leave is_change True; - # that would record change with nothing verified behind it. - raise RuntimeError(f"Output {i} matched but no verification handler applies") - - elif verified_derivation_path is not None and "p2tr" not in self.policy["type"]: + elif verified_derivation_path is not None and self.policy["type"] != "p2tr": # The psbt claims one of this seed's keys on this output, yet the # output does NOT pay what that claim describes. We treat this # deception as an attack. diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index b1ec561a..a61a0b22 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -1888,3 +1888,31 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): with pytest.raises(PSBTOutputOwnershipContradictionError): self._parse(psbt) + + def test__parse__refuses_an_unsupported_script_type(self): + """ + Parsing should be aborted if a psbt has inputs and outputs that use a script type + that embit doesn't recognize. embit reports unhandled types as None. In this case + the output's policy shape would match the input's (`None` == `None`) and so the + parse would consider it as possible change. Rather than continue, we expect the + catch-all safety check to spot the unsupported script type and raise RuntimeError. + """ + root = self._root() + + # embit does not support p2pk so its script type will be `None` + def p2pk(public_key: PublicKey) -> script.Script: + OP_PUSHBYTES_33 = b"\x21" + OP_CHECKSIG = b"\xac" + return script.Script(OP_PUSHBYTES_33 + public_key.sec() + OP_CHECKSIG) + + psbt = self._psbt_with_change() + psbt.inputs[0].witness_utxo.script_pubkey = p2pk(root.derive("m/84h/1h/0h/0/3").get_public_key()) + psbt.outputs[0].script_pubkey = p2pk(root.derive("m/84h/1h/0h/1/0").get_public_key()) + + # The policy shape check really does let this output through to the scriptPubKey + # rebuild step. + assert psbt.inputs[0].witness_utxo.script_pubkey.script_type() is None + assert psbt.outputs[0].script_pubkey.script_type() is None + + with pytest.raises(RuntimeError, match="Unsupported policy type"): + self._parse(psbt) From 321bf14debae9abc1144fafa56f5d3b50691629a Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:13:52 -0500 Subject: [PATCH 4/6] Rename is_change to is_presumed_change The flag in _parse_outputs is set for two different strengths of finding. For single sig our own key rebuilt the scriptPubKey, so the output is provably ours. For multisig we have only shown that our key sits in the committed script, and which wallet that script belongs to is settled later, by the user's descriptor. Calling both "is_change" gave the multisig case a certainty it does not have. The process docstring is held to the same standard: it describes how ownership is determined rather than how outputs are verified as change, the verdict table reads "presumed change", and the multisig rebuild bullet names the key-in-script step that the table row depends on. No behavior change. --- src/seedsigner/models/psbt_parser.py | 32 +++++++++++++++++----------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index f0eaf61d..f995e4ff 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -331,16 +331,17 @@ class PSBTParser(): OP_RETURN data, and totals the amounts for each. Note that self-transfer/receive outputs are also considered "change". - Most of the work here is determining which, if any, outputs are verifiably being - paid to our seed. + Most of the work here is sorting through the psbt's claims about which, if any, + outputs are paying a key that can be derived from our seed, and then doing all + possible independent verifications for the given output data. """ - """****************** How outputs are verified as change ************************ - Many outputs are obviously NOT change. An output is only considered possible + """********************* How output ownership is determined ********************* + Many outputs are obviously NOT ours. An output is only considered possible change if its policy matches the inputs' policy "shape" (script type, plus m-of-n for multisig; see parse()); anything else is recorded as an external spend. - The psbt will usually annotate which key(s) own a change output (see embit's + The psbt will usually annotate which key(s) a change output pays (see embit's bip32_derivations and taproot_bip32_derivations), but this is just a claim supplied by the coordinator. These annotations are not authoritative. But such claims are significant; if our checks prove that the claim is false, we consider @@ -354,7 +355,8 @@ class PSBTParser(): We must build our own version of the scriptPubKey via: * single sig: derive a key from our seed using the claimed derivation path. - * multisig: hash the claimed witness_script or redeem_script. + * multisig: hash the claimed witness_script or redeem_script, then check that a + key derived from our seed is among that script's keys. That leaves us holding two independent answers about the same output: which key it commits to (our rebuild, matched against the scriptPubKey), and which key the psbt @@ -362,12 +364,16 @@ class PSBTParser(): | claims this seed | doesn't claim this seed -------------------------+----------------------+------------------------- - commits to our key | confirmed: is change | contradiction + commits to our key | presumed change | contradiction commits to another key | contradiction | presumed external spend If our two answers contradict each other, the psbt has been caught in a deception. We raise an exception and reject the psbt. + Multisig change can't be fully verified until later in the process, so we use + "presumed" to avoid conveying a false impression of certainty. Single sig carries + its own note later in this function about its guarantees. + (note one exception: no taproot mismatch is rejected. A script tree tweaks our internal key, so an honest taproot change output fails to match too, and we cannot yet tell that apart from an output that claims our key but pays someone else. All @@ -387,7 +393,7 @@ class PSBTParser(): for i, out in enumerate(self.psbt.outputs): out_policy = PSBTParser._get_policy(out, vout[i].script_pubkey, self.psbt.xpubs, child_key_derivation_cache) - is_change = False + is_presumed_change = False # Is this output change? If this output's policy is superficially similar to # the spending wallet's policy (e.g. they're both 2-of-3 p2wsh), then it's a @@ -497,9 +503,9 @@ class PSBTParser(): raise RuntimeError(f"Output {i} verified at a path it does not pay") # We've now verified that the key we derived from our seed at the - # claimed path is the key this output pays. This output is - # provably ours. - is_change = True + # claimed path is the key this output pays. Despite the "presumed" + # variable name, the output IS provably ours. + is_presumed_change = True elif multisig_script is not None: if verified_derivation_path is None: @@ -551,7 +557,7 @@ class PSBTParser(): # multisig that happens to include our seed. Final change # verification can only happen if and when the user loads # their "known-good" multisig descriptor. - is_change = True + is_presumed_change = True elif verified_derivation_path is not None and self.policy["type"] != "p2tr": # The psbt claims one of this seed's keys on this output, yet the @@ -579,7 +585,7 @@ class PSBTParser(): # The data is written as: OP_RETURN + OP_PUSHDATA1 + len(payload) + payload self.op_return_data = vout[i].script_pubkey.data[3:] - elif is_change: + elif is_presumed_change: # Remember that "change" in this function is ANY output coming back to our # seed, receive addresses included. It is up to the View layer to use the # derivation path to determine if it should be displayed as change or From ff1339669189c2d64e5ab9db78e09e3833b7bc61 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Sat, 12 Sep 2026 08:18:59 -0500 Subject: [PATCH 5/6] Compare cosigners before presuming multisig change A multisig output whose committed script holds this seed's key can still belong to a different wallet: a 2-of-3 over {p1,p2,p4} when the inputs spend {p1,p2,p3}. When the psbt supplies global xpubs and fully annotates the output, _get_cosigners resolves a cosigner list for the inputs and one for the output, and lists that differ mean the output pays a quorum other than the one being spent from. That output is now counted as an external spend. It stays presumed change when the lists match or when either side's cosigners are unresolved. This is hygiene, not a security control. The global xpubs are optional and unauthenticated, so a coordinator that omits them skips the check, and a mismatch demotes rather than raises because a transfer into a second multisig this seed belongs to is legitimate. No surveyed coordinator produces the shape the check fires on: every one except Bitcoin Core annotates only its own wallet's outputs, and Core, which annotates any output a descriptor in its wallet file can solve, writes no global xpubs at all. Settling which wallet a multisig output pays still needs the user's own descriptor. The comparison sits after the ownership checks rather than at the policy shape gate, so a psbt whose output cosigners fail to resolve still faces every ownership check. _get_cosigners' sorted() is what makes the two lists comparable: two of the three multisig fixtures order the same wallet's keys differently on the input and on its change output. Its docstring now separates what is claim from what is checked and what the result does and does not establish, its loop carries the procedure inline, and two early-outs name the omitted-xpubs and omitted-derivations cases before the loop (behaviour-neutral: both already raised from inside it). Tests pin the sort, the different-quorum spend, and the no-xpubs presumed-change fallback. --- src/seedsigner/models/psbt_parser.py | 71 ++++++++++- tests/test_psbt_parser.py | 179 +++++++++++++++++++++++++++ 2 files changed, 244 insertions(+), 6 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index f995e4ff..cc2e21b3 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -559,6 +559,19 @@ class PSBTParser(): # their "known-good" multisig descriptor. is_presumed_change = True + # One thing we can rule out now: if the psbt supplied global + # xpubs (see _get_cosigners) AND it fully annotated this + # output, we can see if this output's cosigners differ from + # the inputs' cosigners. If so, then we can be sure this + # output is NOT our change. This sort of mismatch is a + # scenario that no known coordinator would produce, but + # there's no harm in checking this edge case. + input_cosigners = self.policy.get("cosigners") + output_cosigners = out_policy.get("cosigners") + cosigners_resolved = input_cosigners is not None and output_cosigners is not None + if cosigners_resolved and input_cosigners != output_cosigners: + is_presumed_change = False + elif verified_derivation_path is not None and self.policy["type"] != "p2tr": # The psbt claims one of this seed's keys on this output, yet the # output does NOT pay what that claim describes. We treat this @@ -816,25 +829,71 @@ class PSBTParser(): @staticmethod def _get_cosigners(pubkeys, derivations, xpubs, child_key_derivation_cache: dict | None): - """Returns xpubs used to derive pubkeys using global xpub field from psbt""" + """ + Traces every key in a multisig script back to the global xpub it was derived + from, then returns the xpubs it found as a sorted list of base58 strings. + + Args: + * pubkeys: The keys that actually appear in the script (the witness script for + segwit; the redeem script for legacy p2sh). Extracted by _get_policy(). One + per cosigner. + + * derivations: (embit's bip32_derivations) Each pubkey's associated fingerprint + and full derivation path (e.g. m/48'/0'/0'/2'/1/5). A dict keyed on each + pubkey. + + * xpubs: aka "global xpubs". The account-level xpub, with its associated + fingerprint and derivation path, but only down to the account level (e.g. + m/48'/0'/0'/2'). A dict keyed on each xpub. + + The derivations and xpubs are unproven claims provided by the coordinator. So for + each pubkey we check whether the xpub the psbt points us to really derives it. + + The resulting cosigners list consists of each xpub that provably derives each of + the script's keys. But that is ALL it proves. We have no way to verify who those + xpubs actually belong to; the coordinator can list any xpubs it likes. + + The list is sorted so that two scripts holding the same wallet's keys in a + different order resolve to the same cosigners. + + Note that the bip32_derivations and the global xpubs are both optional psbt + fields. If either is omitted or incomplete, this function raises rather than + return a partial list. + """ + # TODO: Improve error handling by providing custom exceptions. + + # Early-out if the optional data is omitted. Not actually an error: raising is + # how this function reports that a complete cosigner list can't be built. + if not xpubs: + raise ValueError("No global xpubs supplied") + if not derivations: + raise ValueError("No derivation paths supplied") + cosigners = [] for i, pubkey in enumerate(pubkeys): + # For each pubkey, get the claimed fingerprint and full derivation path if pubkey not in derivations: raise ValueError("Missing derivation") der = derivations[pubkey] + + # Scan the xpubs for one whose fingerprint and derivation path match the + # claim (xpub path comparisons have to stop at the account level). for xpub in xpubs: origin_der = xpubs[xpub] - # check fingerprint if origin_der.fingerprint == der.fingerprint: - # check derivation - last two indexes give pub from xpub if origin_der.derivation == der.derivation[:-2]: - # check that it derives to pubkey actually - derived_key = PSBTParser._derive_with_cache( - xpub, der.derivation[-2:], child_key_derivation_cache) + # Then derive the actual child key (its full derivation path is + # two indices deeper than the xpub's stated derivation path) + derived_key = PSBTParser._derive_with_cache(xpub, der.derivation[-2:], child_key_derivation_cache) + + # Finally, compare that key with the target pubkey if derived_key.key == pubkey: # append strings so they can be sorted and compared cosigners.append(xpub.to_base58()) break + + # Every key in the script has to trace back to an xpub for the result to mean + # anything. if len(cosigners) != len(pubkeys): raise RuntimeError("Can't get all cosigners") return sorted(cosigners) diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index a61a0b22..cd179832 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -1889,6 +1889,185 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): self._parse(psbt) + def test_get_cosigners_returns_a_sorted_list(self): + """ + Two multisig scripts can list the same wallet's keys in different orders, so the + cosigners resolved for one script and the cosigners resolved for another have to + be sorted before they can be compared. Regression test against _get_cosigners + ever dropping the sort logic. + """ + psbt = PSBT.parse(a2b_base64(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT)) + inp = psbt.inputs[0] + pubkeys = list(inp.bip32_derivations.keys()) + + cosigners = PSBTParser._get_cosigners(pubkeys, inp.bip32_derivations, psbt.xpubs, None) + + assert cosigners == sorted(cosigners) + + # Sanity check: the fixture's three cosigners are three different xpubs. + assert len(set(cosigners)) == 3 + + # The same keys, handed over in the opposite order + reordered = PSBTParser._get_cosigners(list(reversed(pubkeys)), inp.bip32_derivations, psbt.xpubs, None) + + assert reordered == cosigners + + + def _repoint_at_a_different_quorum(self, psbt: PSBT): + """ + Helper function to rebuild the change output's script so it pays a 2-of-3 that + still holds this seed's key, but with one cosigner swapped for a different seed. + That swap makes the output's 2-of-3 a different wallet from the one the inputs + spend from. + """ + # Callers pass a psbt whose change output comes first + out = psbt.outputs[0] + + seed_fingerprint = root_for_seed(self.seed).my_fingerprint + + # The replacement cosigner will use the same account-level derivation path as the + # first xpub. + account_derivation_path = list(psbt.xpubs.values())[0].derivation + + # Pick the first entry that isn't the current seed's. + for public_key, derivation_path_obj in out.bip32_derivations.items(): + if derivation_path_obj.fingerprint != seed_fingerprint: + displaced_public_key = public_key + address_derivation_path = derivation_path_obj.derivation + break + + # Build the outsider from a different seed: its account xpub at the shared + # account path, then the child key two levels down at the displaced entry's + # change/index. + outsider_root = root_for_seed(PSBTTestData.recipient_seed) + outsider_account = outsider_root.derive(account_derivation_path) + outsider_public_key = outsider_account.derive(address_derivation_path[-2:]).get_public_key() + + # Add the outsider's account xpub to the psbt's global xpubs, which now hold + # four: the inputs' three cosigners plus the outsider. + psbt.xpubs[outsider_account.to_public()] = DerivationPath(outsider_root.my_fingerprint, account_derivation_path) + + # Rebuild the multisig script with the outsider's key in the displaced key's + # slot. + original_script = out.witness_script if out.witness_script is not None else out.redeem_script + m, n, public_keys = PSBTParser._parse_multisig(original_script) + new_pubkey_list = [] + for public_key in public_keys: + if public_key == displaced_public_key: + new_pubkey_list.append(outsider_public_key) + else: + new_pubkey_list.append(public_key) + rebuilt_script = script.multisig(m, new_pubkey_list) + + # Swap the displaced cosigner's derivation path entry for the outsider's, so + # the psbt describes the rebuilt script truthfully. + del out.bip32_derivations[displaced_public_key] + out.bip32_derivations[outsider_public_key] = DerivationPath(outsider_root.my_fingerprint, address_derivation_path) + + # Recommit the output to the rebuilt script, through the wrapping the fixture + # uses: p2wsh, p2sh-p2wsh, or bare p2sh. + if out.witness_script is not None: + out.witness_script = rebuilt_script + inner_script = script.p2wsh(rebuilt_script) + else: + inner_script = rebuilt_script + + if out.redeem_script is not None: + out.redeem_script = inner_script + out.script_pubkey = script.p2sh(inner_script) + else: + out.script_pubkey = inner_script + + + def test__parse__counts_a_different_quorum_as_a_spend(self): + """ + An output paying a 2-of-3 that this seed is genuinely part of, but whose third + cosigner is a different seed. That makes the output's 2-of-3 a different wallet + from the one the inputs spend from. + + Every ownership check passes: the output commits to a script holding this seed's + key and the psbt claims this seed there truthfully. + + The psbt supplies global xpubs (the three from the input plus the outsider's that + is part of the output) and it fully annotates this output's derivation paths, + allowing for the parser to determine the output's cosigners accurately. + + End result of this setup: The output's cosigner list differs from the inputs' + list. So the output is counted as a spend. + + This sort of cosigner mismatch is a scenario that no known coordinator would + produce; normally an output to a different wallet would not be annotated nor have + its xpubs added to the global xpubs data. + """ + for input_base64, change_hex in [ + (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE), + ]: + # The wallet's own change output, for comparison + psbt_parser = self._parse(self._psbt_with_change(input_base64, change_hex)) + assert psbt_parser.change_amount == 10_000 + assert psbt_parser.spend_amount == 0 + + psbt = self._psbt_with_change(input_base64, change_hex) + self._repoint_at_a_different_quorum(psbt) + + # The parse accepts the psbt: it described this output accurately. + psbt_parser = self._parse(psbt) + + # This seed's key really is in the committed script and the psbt's claim of + # this seed verified. + assert psbt_parser.verified_output_derivation_paths[0] is not None + + # But the output pays a different quorum than the inputs spend from, so it + # is counted as a spend. + assert psbt_parser.change_data == [] + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + + def test__parse__counts_a_different_quorum_as_change_if_no_global_xpubs(self): + """ + Same setup as the previous test, but this time the psbt omits its global xpubs. + + The global xpubs are needed in order to resolve cosigners. So without them, the + inputs' cosigner list and the output's cosigner list comparison is skipped. The + user's seed is part of the output wallet and the output's policy "shape" + superficially matches the input's (2-of-3, same script type), so the output is + counted as presumed change. + + BIP-174 makes the global xpubs optional and honest coordinators do omit them. The + previous test notes that no known coordinator annotates external spend outputs so + this test scenario is unlikely to be seen in the real world. But this version of + the test has one notable exception: Bitcoin Core. + + Core can hold the descriptors of several spending wallets. It will annotate an + output that belongs to ANY of its descriptors, regardless of whether it differs + from the input wallet. But Core does not write global xpubs at all, so it wasn't a + factor in the previous test (which required the global xpubs). + + But a Core-built psbt can exactly match this test's shape: a fully annotated + foreign output and no global xpubs to compare against. + """ + # Test each multisig script type + for input_base64, change_hex in [ + (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE), + ]: + psbt = self._psbt_with_change(input_base64, change_hex) + self._repoint_at_a_different_quorum(psbt) + + # The global xpubs must be omitted for this scenario + psbt.xpubs.clear() + + psbt_parser = self._parse(psbt) + + # Parser categorizes the output as presumed change. + assert psbt_parser.change_amount == 10_000 + assert psbt_parser.spend_amount == 0 + + def test__parse__refuses_an_unsupported_script_type(self): """ Parsing should be aborted if a psbt has inputs and outputs that use a script type From 45a5eabb9d3bb8afc496d219168642845c4cc8a1 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Wed, 16 Sep 2026 09:30:56 -0500 Subject: [PATCH 6/6] Count an unresolved output cosigner set as a mismatch The cosigner comparison skipped any output whose cosigners failed to resolve, so the different-quorum case it exists for never fired on a real psbt: coordinators write only the spending wallet's xpubs, and a foreign cosigner's key then resolves against nothing. Once the inputs' cosigners resolve, an output that differs or fails to resolve is counted as a spend. Keys now match xpubs by derivation alone rather than requiring the claimed fingerprint to match first. The fingerprint is a label the coordinator wrote; requiring it let one mislabeled cosigner turn genuine change into a spend, and a key an xpub really derives is proof enough on its own. The different-quorum test now uses the wallet's own xpubs, the shape every xpub-writing coordinator produces, and a second test keeps the four-xpub variant. --- src/seedsigner/models/psbt_parser.py | 43 ++++++----- tests/test_psbt_parser.py | 103 +++++++++++++++++++++------ 2 files changed, 101 insertions(+), 45 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index cc2e21b3..441c1e1d 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -560,16 +560,14 @@ class PSBTParser(): is_presumed_change = True # One thing we can rule out now: if the psbt supplied global - # xpubs (see _get_cosigners) AND it fully annotated this - # output, we can see if this output's cosigners differ from - # the inputs' cosigners. If so, then we can be sure this - # output is NOT our change. This sort of mismatch is a - # scenario that no known coordinator would produce, but - # there's no harm in checking this edge case. + # xpubs (see _get_cosigners), we can compare this output's + # cosigners to the inputs' cosigners. Real change should have + # the same cosigners; if this output's cosigners differ or + # fail to resolve at all, we classify this output as NOT + # change. input_cosigners = self.policy.get("cosigners") output_cosigners = out_policy.get("cosigners") - cosigners_resolved = input_cosigners is not None and output_cosigners is not None - if cosigners_resolved and input_cosigners != output_cosigners: + if input_cosigners is not None and input_cosigners != output_cosigners: is_presumed_change = False elif verified_derivation_path is not None and self.policy["type"] != "p2tr": @@ -846,8 +844,9 @@ class PSBTParser(): fingerprint and derivation path, but only down to the account level (e.g. m/48'/0'/0'/2'). A dict keyed on each xpub. - The derivations and xpubs are unproven claims provided by the coordinator. So for - each pubkey we check whether the xpub the psbt points us to really derives it. + The derivations and xpubs are unproven claims provided by the coordinator. So we + take each pubkey's claimed derivation path and check whether one of the xpubs + really derives that pubkey. The resulting cosigners list consists of each xpub that provably derives each of the script's keys. But that is ALL it proves. We have no way to verify who those @@ -876,21 +875,21 @@ class PSBTParser(): raise ValueError("Missing derivation") der = derivations[pubkey] - # Scan the xpubs for one whose fingerprint and derivation path match the - # claim (xpub path comparisons have to stop at the account level). + # Scan the xpubs for one whose derivation path matches the claim. for xpub in xpubs: origin_der = xpubs[xpub] - if origin_der.fingerprint == der.fingerprint: - if origin_der.derivation == der.derivation[:-2]: - # Then derive the actual child key (its full derivation path is - # two indices deeper than the xpub's stated derivation path) - derived_key = PSBTParser._derive_with_cache(xpub, der.derivation[-2:], child_key_derivation_cache) + # The full derivation path goes two indices deeper than the xpub's so we + # omit those last two when comparing. + if origin_der.derivation == der.derivation[:-2]: + # Derive the child key that sits two indices below the xpub (i.e. at + # the full derivation path). + derived_key = PSBTParser._derive_with_cache(xpub, der.derivation[-2:], child_key_derivation_cache) - # Finally, compare that key with the target pubkey - if derived_key.key == pubkey: - # append strings so they can be sorted and compared - cosigners.append(xpub.to_base58()) - break + # Finally, compare that key with the target pubkey + if derived_key.key == pubkey: + # Append as strings so they can be sorted and compared + cosigners.append(xpub.to_base58()) + break # Every key in the script has to trace back to an xpub for the result to mean # anything. diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index cd179832..4d28c2ff 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -1919,6 +1919,10 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): still holds this seed's key, but with one cosigner swapped for a different seed. That swap makes the output's 2-of-3 a different wallet from the one the inputs spend from. + + The psbt's global xpubs are left as the fixture wrote them: the inputs' three + cosigners (the expected behavior for all known coordinators). The outsider's xpub + is not added to the global xpubs. """ # Callers pass a psbt whose change output comes first out = psbt.outputs[0] @@ -1943,10 +1947,6 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): outsider_account = outsider_root.derive(account_derivation_path) outsider_public_key = outsider_account.derive(address_derivation_path[-2:]).get_public_key() - # Add the outsider's account xpub to the psbt's global xpubs, which now hold - # four: the inputs' three cosigners plus the outsider. - psbt.xpubs[outsider_account.to_public()] = DerivationPath(outsider_root.my_fingerprint, account_derivation_path) - # Rebuild the multisig script with the outsider's key in the displaced key's # slot. original_script = out.witness_script if out.witness_script is not None else out.redeem_script @@ -1988,16 +1988,23 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): Every ownership check passes: the output commits to a script holding this seed's key and the psbt claims this seed there truthfully. - The psbt supplies global xpubs (the three from the input plus the outsider's that - is part of the output) and it fully annotates this output's derivation paths, - allowing for the parser to determine the output's cosigners accurately. + The psbt supplies the global xpubs of the wallet the inputs spend from. The output + is fully annotated so the parser tries to trace each of the output's three keys + back to one of those xpubs. The two keys it shares with the inputs resolve, but no + xpub derives the outsider's key. So the output is counted as a spend. - End result of this setup: The output's cosigner list differs from the inputs' - list. So the output is counted as a spend. + tldr: different output quorum + global xpubs + annotated external output - This sort of cosigner mismatch is a scenario that no known coordinator would - produce; normally an output to a different wallet would not be annotated nor have - its xpubs added to the global xpubs data. + No known coordinator provides the global xpubs AND annotates an output paying a + different wallet. + * Every coordinator that writes global xpubs: annotates only its own wallet's + outputs. + * Bitcoin Core: annotates an output paying any wallet in its wallet file, but + writes no global xpubs. + * Note: a coordinator may exclude both global xpubs and all output annotations. + Such coordinators are irrelevant for this test. + + So the check exists to catch buggy software or a maliciously edited psbt. """ for input_base64, change_hex in [ (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), @@ -2026,27 +2033,77 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): assert psbt_parser.spend_amount == 10_000 + def test__parse__counts_a_different_quorum_as_a_spend_when_its_xpubs_are_supplied(self): + """ + Same setup as the previous test, but this time the psbt's global xpubs also hold + the outsider's account xpub. So the parser traces every key in the output back to + an xpub, but the output's cosigners resolve to a list that differs from the + inputs' cosigners. The output is counted as a spend. + + tldr: different output quorum + external xpub IN global xpubs + annotated external + output + + No known coordinator adds an external wallet's xpub to the global xpubs, so this + is not expected to be seen in the real world unless someone manually edits a psbt + to include it. + """ + for input_base64, change_hex in [ + (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE), + (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE), + ]: + psbt = self._psbt_with_change(input_base64, change_hex) + self._repoint_at_a_different_quorum(psbt) + + # Add the outsider's account xpub to the psbt's global xpubs, at the same + # account-level derivation path the helper used. The global xpubs now hold + # four: the inputs' three cosigners plus the outsider. + account_derivation_path = list(psbt.xpubs.values())[0].derivation + outsider_root = root_for_seed(PSBTTestData.recipient_seed) + outsider_account = outsider_root.derive(account_derivation_path) + psbt.xpubs[outsider_account.to_public()] = DerivationPath(outsider_root.my_fingerprint, account_derivation_path) + assert len(psbt.xpubs) == 4 + + psbt_parser = self._parse(psbt) + + # Sanity check the setup: the output's cosigners resolve and differ from the + # inputs' cosigners. The parser keeps only the inputs' policy, so the output's + # is rebuilt here the same way the parser does it. + out = psbt.outputs[0] + out_policy = PSBTParser._get_policy(out, out.script_pubkey, psbt.xpubs, None) + input_cosigners = psbt_parser.policy["cosigners"] + output_cosigners = out_policy["cosigners"] + assert len(input_cosigners) == 3 + assert len(output_cosigners) == 3 + assert input_cosigners != output_cosigners + + # The output should be counted as a spend. + assert psbt_parser.change_data == [] + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + def test__parse__counts_a_different_quorum_as_change_if_no_global_xpubs(self): """ - Same setup as the previous test, but this time the psbt omits its global xpubs. + Another test variation: output is once again paying a different quorum (one new + external xpub replacing one of the inputs' cosigners), but this time the psbt + omits its global xpubs. The global xpubs are needed in order to resolve cosigners. So without them, the inputs' cosigner list and the output's cosigner list comparison is skipped. The - user's seed is part of the output wallet and the output's policy "shape" + user's seed IS part of the output wallet and the output's policy "shape" superficially matches the input's (2-of-3, same script type), so the output is counted as presumed change. - BIP-174 makes the global xpubs optional and honest coordinators do omit them. The - previous test notes that no known coordinator annotates external spend outputs so - this test scenario is unlikely to be seen in the real world. But this version of - the test has one notable exception: Bitcoin Core. + tldr: different output quorum + NO global xpubs + annotated external output - Core can hold the descriptors of several spending wallets. It will annotate an - output that belongs to ANY of its descriptors, regardless of whether it differs - from the input wallet. But Core does not write global xpubs at all, so it wasn't a - factor in the previous test (which required the global xpubs). + BIP-174 makes the global xpubs optional and honest coordinators do omit them. - But a Core-built psbt can exactly match this test's shape: a fully annotated + Bitcoin Core can hold the descriptors of several spending wallets and will + annotate an output that belongs to ANY of its descriptors, regardless of whether + it differs from the input wallet. And Core never provides the global xpubs. + + So a Core-built psbt can exactly match this test's shape: a fully annotated foreign output and no global xpubs to compare against. """ # Test each multisig script type