From 98b46f5a2921bcabe0c1bb9bbf3d501a9b3f971d Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:41:48 -0500 Subject: [PATCH 1/6] Rename _derive_with_cache to _derive_with_cache_via_indices Names the input shape: a list of child indices below parent_key, which is what the cache is keyed on. A companion that takes a psbt entry's DerivationPath object follows; the two names then say which one a caller holds. --- src/seedsigner/models/psbt_parser.py | 16 ++++++++-------- tests/test_psbt_parser.py | 21 +++++++++++---------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 441c1e1db..a993b1bfa 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -267,7 +267,7 @@ def parse(self): levels, differing only in the address at the end. So every level derived during this parse is kept in a cache and reused. See - _derive_with_cache. + _derive_with_cache_via_indices. Note that the cache is only useful within a single parse so it is not preserved. """ @@ -437,7 +437,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # 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() + seed_public_key = PSBTParser._derive_with_cache_via_indices(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 @@ -464,7 +464,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): 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() + seed_public_key = PSBTParser._derive_with_cache_via_indices(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 @@ -517,7 +517,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # 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() + seed_public_key = PSBTParser._derive_with_cache_via_indices(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 @@ -535,7 +535,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # 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() + seed_public_key = PSBTParser._derive_with_cache_via_indices(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 @@ -775,7 +775,7 @@ def _multisig_script_contains_key(multisig_script: script.Script, public_key: Pu @staticmethod - def _derive_with_cache(parent_key: bip32.HDKey, derivation_path: List[int], child_key_derivation_cache: dict | None = None) -> bip32.HDKey: + def _derive_with_cache_via_indices(parent_key: bip32.HDKey, derivation_path: List[int], child_key_derivation_cache: dict | None = None) -> bip32.HDKey: """ Derives the key that sits at the given derivation path below parent_key, reusing any levels along the way that have already been derived during this parse. @@ -883,7 +883,7 @@ def _get_cosigners(pubkeys, derivations, xpubs, child_key_derivation_cache: dict 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) + derived_key = PSBTParser._derive_with_cache_via_indices(xpub, der.derivation[-2:], child_key_derivation_cache) # Finally, compare that key with the target pubkey if derived_key.key == pubkey: @@ -975,7 +975,7 @@ def seed_owns_pubkey(root: bip32.HDKey, claimed_derivation_path: List[int], publ say anything. Ownership is established here and only here, by deriving the key again from the seed and comparing the actual key material. """ - derived_public_key = PSBTParser._derive_with_cache(root, claimed_derivation_path, child_key_derivation_cache).get_public_key() + derived_public_key = PSBTParser._derive_with_cache_via_indices(root, claimed_derivation_path, child_key_derivation_cache).get_public_key() if is_taproot: # A psbt carries a taproot key as its bare 32-byte x coordinate, but embit diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index 4d28c2ff6..8acd7e5d1 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -569,13 +569,14 @@ def assert_same_parse_result(self, parser_a: PSBTParser, parser_b: PSBTParser): def cache_size_recorder(self, cache_sizes: list): """ - Returns a stand-in for _derive_with_cache that derives exactly as the real one - does, but appends the cache's size to cache_sizes on the way out of every call. + Returns a stand-in for _derive_with_cache_via_indices that derives exactly as the + real one does, but appends the cache's size to cache_sizes on the way out of every + call. The cache is a local inside parse(), so intercepting the calls it gets handed to is the only way to see how large it grew. """ - real_derive_with_cache = PSBTParser._derive_with_cache + real_derive_with_cache = PSBTParser._derive_with_cache_via_indices def recorded(parent_key, derivation_path, cache=None): derived_key = real_derive_with_cache(parent_key, derivation_path, cache) @@ -671,8 +672,8 @@ def test_derive_with_cache_does_not_cross_parent_keys(self): # The cache here isn't providing any speedup (there are no derivations in the # cache to take advantage of), but we're just testing that the cache doesn't # confuse/combine the two cosigners' derivation data. - from_a = PSBTParser._derive_with_cache(cosigner_a_xpub, receive_index_5, cache) - from_b = PSBTParser._derive_with_cache(cosigner_b_xpub, receive_index_5, cache) + from_a = PSBTParser._derive_with_cache_via_indices(cosigner_a_xpub, receive_index_5, cache) + from_b = PSBTParser._derive_with_cache_via_indices(cosigner_b_xpub, receive_index_5, cache) # Two levels should have been added for each cosigner assert len(cache) == 4 @@ -738,7 +739,7 @@ def build_psbt(input_base64: str, change_hex: str) -> PSBT: def assert_cache_makes_no_difference(input_base64: str, change_hex: str): # Store the real function before the patches below replace it. Each replacement # still needs access to the real function to do the actual deriving. - real_derive_with_cache = PSBTParser._derive_with_cache + real_derive_with_cache = PSBTParser._derive_with_cache_via_indices # This version of the replacement will derive exactly as the real cache-backed # function does, but will also record the cache it was handed on each call. @@ -747,7 +748,7 @@ def recording_derive_with_cache(parent_key, derivation_path, cache=None): caches_received.append(cache) return real_derive_with_cache(parent_key, derivation_path, cache) - with patch.object(PSBTParser, "_derive_with_cache", staticmethod(recording_derive_with_cache)): + with patch.object(PSBTParser, "_derive_with_cache_via_indices", staticmethod(recording_derive_with_cache)): with_cache = PSBTParser( build_psbt(input_base64, change_hex), self.seed, network=SettingsConstants.REGTEST) @@ -759,7 +760,7 @@ def recording_derive_with_cache(parent_key, derivation_path, cache=None): def cache_free_derive(parent_key, derivation_path, cache=None): return real_derive_with_cache(parent_key, derivation_path) - with patch.object(PSBTParser, "_derive_with_cache", staticmethod(cache_free_derive)): + with patch.object(PSBTParser, "_derive_with_cache_via_indices", staticmethod(cache_free_derive)): without_cache = PSBTParser( build_psbt(input_base64, change_hex), self.seed, network=SettingsConstants.REGTEST) @@ -791,7 +792,7 @@ def build_psbt(case: tuple) -> PSBT: # Record how large the cache grew over the course of each parse unconstrained_sizes = [] - with patch.object(PSBTParser, "_derive_with_cache", staticmethod(self.cache_size_recorder(unconstrained_sizes))): + with patch.object(PSBTParser, "_derive_with_cache_via_indices", staticmethod(self.cache_size_recorder(unconstrained_sizes))): multisig_unconstrained = PSBTParser(build_psbt(multisig_case), self.seed, network=SettingsConstants.REGTEST) singlesig_unconstrained = PSBTParser(build_psbt(singlesig_case), self.seed, network=SettingsConstants.REGTEST) @@ -800,7 +801,7 @@ def build_psbt(case: tuple) -> PSBT: cap = 3 capped_sizes = [] with patch.object(PSBTParser, "MAX_CACHED_DERIVATIONS", cap): - with patch.object(PSBTParser, "_derive_with_cache", staticmethod(self.cache_size_recorder(capped_sizes))): + with patch.object(PSBTParser, "_derive_with_cache_via_indices", staticmethod(self.cache_size_recorder(capped_sizes))): multisig_capped = PSBTParser(build_psbt(multisig_case), self.seed, network=SettingsConstants.REGTEST) singlesig_capped = PSBTParser(build_psbt(singlesig_case), self.seed, network=SettingsConstants.REGTEST) From e1cd05dceb17181520eefd61ec897eb74efdd18b Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:41:48 -0500 Subject: [PATCH 2/6] Hold every claim of ours on a multisig output to its script A multisig output lists one derivation path entry per cosigner. The output check verified only the first entry claiming this seed against the committed script, so a second claim of ours whose key the script has no use for went unreported when listed behind our real entry or in the place of another cosigner's entry (which keeps the entry count at n and passes the surplus count). Every entry claiming this seed is now held to the script. To feed that, the ownership scan keeps every entry it proved per input and output, as the psbt's own DerivationPath objects, instead of the first one's path. _derive_with_cache_via_derivation_path is added beside the index-taking primitive for callers holding such an entry; it reads only the entry's path, since the single-sig rebuild and the multisig fallback derive at entries carrying a foreign fingerprint on purpose. change_data keeps the index-list path the views read. --- src/seedsigner/models/psbt_parser.py | 119 ++++++++------ tests/test_psbt_parser.py | 222 +++++++++++++++++---------- 2 files changed, 209 insertions(+), 132 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index a993b1bfa..fffed76e2 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -174,10 +174,11 @@ def __init__(self, p: PSBT, seed: Seed, network: str = SettingsConstants.MAINNET self.op_return_data: bytes = None # Contains one entry per input in psbt.inputs and per output in psbt.outputs. Each - # entry is either the derivation path the seed genuinely owns there, or it is set - # to `None`. - self.verified_input_derivation_paths: List[List[int] | None] = [] - self.verified_output_derivation_paths: List[List[int] | None] = [] + # entry lists every derivation path the seed genuinely owns there, in the order + # the psbt lists them. An input or output that does not claim any of our keys + # gets an empty list. + self.verified_input_derivation_paths: List[List[DerivationPath]] = [] + self.verified_output_derivation_paths: List[List[DerivationPath]] = [] self.root = None @@ -246,8 +247,8 @@ def parse(self): 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. + - multisig: Match each of the seed's verified keys 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. @@ -436,8 +437,8 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # 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_via_indices(self.root, singlesig_derivation_path, child_key_derivation_cache).get_public_key() + singlesig_derivation_path = list(out.bip32_derivations.values())[0] + seed_public_key = PSBTParser._derive_with_cache_via_derivation_path(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 @@ -462,9 +463,8 @@ def _parse_outputs(self, child_key_derivation_cache: dict): 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_via_indices(self.root, singlesig_derivation_path, child_key_derivation_cache).get_public_key() + leaf_hashes, singlesig_derivation_path = taproot_entries[0] + seed_public_key = PSBTParser._derive_with_cache_via_derivation_path(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 @@ -483,19 +483,19 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # which is also caught here. raise RuntimeError(f"Unsupported policy type: {self.policy['type']}") - verified_derivation_path = self.verified_output_derivation_paths[i] + verified_derivation_paths = 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. if singlesig_derivation_path is not None: - if verified_derivation_path is None: + if verified_derivation_paths == []: # 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") + raise PSBTOutputOwnershipContradictionError(f"Output pays this seed at {bip32.path_to_str(singlesig_derivation_path.derivation)} but does not claim it there") - if verified_derivation_path != singlesig_derivation_path: + if verified_derivation_paths != [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 @@ -508,7 +508,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): is_presumed_change = True elif multisig_script is not None: - if verified_derivation_path is None: + if verified_derivation_paths == []: # 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. @@ -517,7 +517,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # 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_via_indices(self.root, derivation_path_obj.derivation, child_key_derivation_cache).get_public_key() + seed_public_key = PSBTParser._derive_with_cache_via_derivation_path(self.root, derivation_path_obj, 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 @@ -533,14 +533,17 @@ def _parse_outputs(self, child_key_derivation_cache: dict): 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_via_indices(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") + # multisig, at one or more specific paths. So now we verify + # that the key at every claimed path is in the committed + # script. + for verified_derivation_path in verified_derivation_paths: + seed_public_key = PSBTParser._derive_with_cache_via_derivation_path(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.derivation)} 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 @@ -570,7 +573,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): 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": + elif verified_derivation_paths != [] 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. @@ -590,7 +593,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # 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") + raise PSBTOutputOwnershipContradictionError(f"Output claims this seed at {bip32.path_to_str(verified_derivation_paths[0].derivation)} 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 @@ -606,7 +609,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): "output_index": i, "address": addr, "amount": vout[i].value, - "verified_derivation_path": self.verified_output_derivation_paths[i], + "verified_derivation_path": self.verified_output_derivation_paths[i][0].derivation, }) self.change_amount += vout[i].value @@ -825,6 +828,19 @@ def _derive_with_cache_via_indices(parent_key: bip32.HDKey, derivation_path: Lis return derived_key + @staticmethod + def _derive_with_cache_via_derivation_path(parent_key: bip32.HDKey, derivation_path: DerivationPath, child_key_derivation_cache: dict | None = None) -> bip32.HDKey: + """ + _derive_with_cache_via_indices for a psbt entry: derives at the entry's full + derivation path below parent_key. + + The DerivationPath.fingerprint is completely ignored; this function allows for + deriving a key even when it's known that the fingerprint doesn't match (e.g. to + catch a false claim). + """ + return PSBTParser._derive_with_cache_via_indices(parent_key, derivation_path.derivation, child_key_derivation_cache) + + @staticmethod def _get_cosigners(pubkeys, derivations, xpubs, child_key_derivation_cache: dict | None): """ @@ -996,12 +1012,13 @@ def seed_owns_pubkey(root: bip32.HDKey, claimed_derivation_path: List[int], publ @staticmethod - def _get_seed_derivation_path(scope: InputScope | OutputScope, root: bip32.HDKey, child_key_derivation_cache: dict) -> List[int] | None: + def _get_seed_derivation_paths(scope: InputScope | OutputScope, root: bip32.HDKey, child_key_derivation_cache: dict) -> List[DerivationPath]: """ Scans the derivation path(s) in the provided input or output scope to determine which, if any, are provably derived from the signing seed (for multisig a path is provided per key; if the seed is part of the multisig, one of the n paths will - match). Returns the verified derivation path (as a list of ints) or None. + match). Returns every verified DerivationPath entry, in the order the psbt lists + them. An input or output that does not claim any of our keys yields an empty list. Every key in the scope that claims this seed's fingerprint is re-derived and checked. A false claim raises PSBT[Output|Input]OwnershipClaimError. @@ -1015,22 +1032,26 @@ def _get_seed_derivation_path(scope: InputScope | OutputScope, root: bip32.HDKey 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 - verify against this seed; we return the first but still check the rest. + One edge case: A scope may carry more than one entry that verifies against this + seed. + * Foolish as it may be, a multisig could honestly use this seed in two cosigner + slots, each at its own derivation path. + * More importantly: a malicious psbt could list a second claim of ours as a + decoy, at a path our seed really does derive but whose key the committed + script has no use for. + + This function only checks and returns the DerivationPath entry for each key that + derives from our seed. What those entries mean for the psbt is determined + elsewhere. The path itself is still whatever the psbt supplied: it can be any length or shape, since any path that derives from the seed will pass. Whether the path is - one the user's wallet would ever look at is a separate question, answered - elsewhere. + one the user's wallet would ever look at is a separate question. """ seed_fingerprint = root.my_fingerprint - verified_derivation_path = None + verified_derivation_paths = [] def _check_claim(public_key: PublicKey, derivation_path_obj: DerivationPath, is_taproot: bool): - nonlocal verified_derivation_path - if derivation_path_obj.fingerprint != seed_fingerprint: # Claims to belong to some other key. Nothing to prove or disprove here. return @@ -1039,9 +1060,7 @@ def _check_claim(public_key: PublicKey, derivation_path_obj: DerivationPath, is_ error_class = (PSBTInputOwnershipClaimError if isinstance(scope, InputScope) else PSBTOutputOwnershipClaimError) raise error_class(f"Key at {bip32.path_to_str(derivation_path_obj.derivation)} claims this seed's fingerprint but does not derive from it") - # Store only the first verified path - if verified_derivation_path is None: - verified_derivation_path = derivation_path_obj.derivation + verified_derivation_paths.append(derivation_path_obj) # Note that both loops check EVERY claim for public_key, derivation_path_obj in scope.bip32_derivations.items(): @@ -1057,14 +1076,15 @@ def _check_claim(public_key: PublicKey, derivation_path_obj: DerivationPath, is_ if scope.bip32_derivations and scope.taproot_bip32_derivations: raise PSBTMixedDerivationPathTypesError("Scope declares both ecdsa and taproot derivation paths") - return verified_derivation_path + return verified_derivation_paths def _verify_claimed_derivation_paths(self, child_key_derivation_cache: dict): """ 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 (no entry claimed this seed) for each input/output scope. + result, stored in verified_[input|output]_derivation_paths, is the list of + verified DerivationPath entries for each input/output scope (empty where no entry + claimed this seed). 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. @@ -1076,12 +1096,12 @@ def _verify_claimed_derivation_paths(self, child_key_derivation_cache: dict): Raises PSBT[Output|Input]OwnershipClaimError on the first false claim detected. """ self.verified_output_derivation_paths = [ - PSBTParser._get_seed_derivation_path(out, self.root, child_key_derivation_cache) + PSBTParser._get_seed_derivation_paths(out, self.root, child_key_derivation_cache) for out in self.psbt.outputs ] self.verified_input_derivation_paths = [ - PSBTParser._get_seed_derivation_path(inp, self.root, child_key_derivation_cache) + PSBTParser._get_seed_derivation_paths(inp, self.root, child_key_derivation_cache) for inp in self.psbt.inputs ] @@ -1107,8 +1127,9 @@ def _reject_if_seed_cannot_sign(self): # 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. - if any(path is not None for path in self.verified_input_derivation_paths): - return + for verified_derivation_paths in self.verified_input_derivation_paths: + if verified_derivation_paths != []: + return # There's nothing for this seed to sign raise PSBTSeedCannotSignError() diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index 8acd7e5d1..ad82abfef 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -270,7 +270,7 @@ def test_missing_fingerprint_handling(self): parser = PSBTParser(p=psbt, seed=PSBTTestData.seed, network=SettingsConstants.REGTEST) (_, filled_derivation) = parser.psbt.inputs[0].taproot_bip32_derivations[x_only_public_key] assert filled_derivation.fingerprint == parser.root.my_fingerprint - assert parser.verified_input_derivation_paths == [bip32.parse_path(odd_parity_derivation_path)] + assert parser.verified_input_derivation_paths == [[filled_derivation]] def test_trim_and_sig_count(self): @@ -687,6 +687,26 @@ def test_derive_with_cache_does_not_cross_parent_keys(self): assert from_b.key.sec() == cosigner_b_xpub.derive(receive_index_5).key.sec() + def test_derive_with_cache_via_derivation_path_ignores_the_entry_fingerprint(self): + """ + The helper derives the key at the entry's path, whatever fingerprint the entry + lists. + """ + root = self._root() + derivation_path = bip32.parse_path("m/84h/1h/0h/1/7") + expected_key = root.derive(derivation_path).key.sec() + + # Our own fingerprint, the all-zero placeholder, and a stranger's + for fingerprint in [root.my_fingerprint, b"\x00\x00\x00\x00", bytes.fromhex("deadbeef")]: + entry = DerivationPath(fingerprint, derivation_path) + derived = PSBTParser._derive_with_cache_via_derivation_path(root, entry, {}) + assert derived.key.sec() == expected_key + + # Same answer with the cache disabled + derived = PSBTParser._derive_with_cache_via_derivation_path(root, entry, None) + assert derived.key.sec() == expected_key + + def test_get_cosigners_identical_with_and_without_cache(self): """ The cache is transparent to callers: _get_cosigners returns the same cosigner @@ -718,9 +738,9 @@ def test_get_cosigners_identical_with_and_without_cache(self): def test_cache_does_not_change_parse_output(self): """ The whole point of the cache is that it changes nothing at all. Parse the same - psbt twice — once normally, once with the cache discarded so that every derivation - falls through to embit's own HDKey.derive() — and require identical parser state - and identical resulting psbt bytes. + psbt twice: once normally, once with the cache discarded so that every derivation + falls through to embit's own HDKey.derive(). The two runs must yield the identical + parser state and resulting psbt bytes. Single-sig and multisig each get a run because they reach the cache from different starting points: single-sig traverses down from our own root, multisig down from @@ -844,6 +864,9 @@ def _psbt_with_change(self, input_base64: str = None, change_hex: str = None) -> def _parse(self, psbt: PSBT) -> PSBTParser: + # TODO: Rename this helper. "parse" does not convey that a new PSBTParser instance + # is being created and it creates confusion in tests that also call PSBT.parse() + # (embit's deserializer). return PSBTParser(psbt, self.seed, network=SettingsConstants.REGTEST) @@ -928,20 +951,20 @@ def test__parse__populates_verified_derivation_paths(self): assert len(psbt_parser.verified_output_derivation_paths) == len(psbt.outputs) # Every recorded path is one the seed really does derive the scope's key at - for scopes, verified_derivation_paths in [ + for scopes, verified_derivation_paths_per_scope in [ (psbt.inputs, psbt_parser.verified_input_derivation_paths), (psbt.outputs, psbt_parser.verified_output_derivation_paths), ]: - for scope, verified_derivation_path in zip(scopes, verified_derivation_paths): - assert verified_derivation_path is not None + for scope, verified_derivation_paths in zip(scopes, verified_derivation_paths_per_scope): + assert len(verified_derivation_paths) == 1 public_key = list(scope.bip32_derivations.keys())[0] - assert PSBTParser.seed_owns_pubkey(psbt_parser.root, verified_derivation_path, public_key, child_key_derivation_cache=None) is True + assert PSBTParser.seed_owns_pubkey(psbt_parser.root, verified_derivation_paths[0].derivation, public_key, child_key_derivation_cache=None) is True - def test__parse__verified_derivation_paths_none_for_not_owned_output(self): + def test__parse__verified_derivation_paths_empty_for_not_owned_output(self): """ An output paying someone else is not a failure; the seed simply owns nothing - there so the matching verified_output_derivation_paths should be None. + there so the matching verified_output_derivation_paths entry should be empty. """ psbt = self._psbt_with_change() @@ -950,11 +973,11 @@ def test__parse__verified_derivation_paths_none_for_not_owned_output(self): psbt_parser = self._parse(psbt) - assert psbt_parser.verified_output_derivation_paths[0] is None - assert psbt_parser.verified_output_derivation_paths[1] is not None + assert psbt_parser.verified_output_derivation_paths[0] == [] + assert psbt_parser.verified_output_derivation_paths[1] != [] - def test__parse__verified_derivation_paths_none_for_not_owned_input(self): + def test__parse__verified_derivation_paths_empty_for_not_owned_input(self): """ A collaborative spend also includes an input belonging to another party, in two shapes: a payjoin counterparty's input arrives finalized with no derivation info @@ -972,15 +995,15 @@ def test__parse__verified_derivation_paths_none_for_not_owned_input(self): # The payjoin shape psbt_parser = self._parse(psbt) - assert psbt_parser.verified_input_derivation_paths[0] is not None - assert psbt_parser.verified_input_derivation_paths[1] is None + assert psbt_parser.verified_input_derivation_paths[0] != [] + assert psbt_parser.verified_input_derivation_paths[1] == [] # The coordinated shape: the derivation entry is truthful, naming the other # party's fingerprint and a key that party really controls. claim_seed_owns_key(foreign_input, "m/84h/1h/0h/0/0", foreign_public_key(), seed=PSBTTestData.recipient_seed) psbt_parser = self._parse(psbt) - assert psbt_parser.verified_input_derivation_paths[0] is not None - assert psbt_parser.verified_input_derivation_paths[1] is None + assert psbt_parser.verified_input_derivation_paths[0] != [] + assert psbt_parser.verified_input_derivation_paths[1] == [] def test__parse__rejects_a_forged_claim_on_an_input(self): @@ -1113,7 +1136,7 @@ def test__parse__accepts_a_key_belonging_to_someone_else(self): # The seed still owns its own key in that input, via the scope's genuine # derivation. - assert psbt_parser.verified_input_derivation_paths[0] is not None + assert psbt_parser.verified_input_derivation_paths[0] != [] def test_genuine_fingerprint_collision_is_rejected_like_a_forgery(self): @@ -1176,8 +1199,9 @@ def counting_child(self, index, hardened=False): # Sanity check: the scan really did run over all ten inputs and the change output assert len(psbt_parser.verified_input_derivation_paths) == 10 - assert all(path is not None for path in psbt_parser.verified_input_derivation_paths) - assert psbt_parser.verified_output_derivation_paths[0] is not None + for verified_derivation_paths in psbt_parser.verified_input_derivation_paths: + assert verified_derivation_paths != [] + assert psbt_parser.verified_output_derivation_paths[0] != [] # The inputs were cloned so they all use the same path with num_levels depth. The # change output differs only in its last two levels. Verify that each of these @@ -1211,14 +1235,10 @@ def test__parse__accepts_a_cosigner_seed_on_a_multisig(self): """ psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE) - psbt_parser = PSBTParser(psbt, PSBTTestData.seed, network=SettingsConstants.REGTEST) - assert any(path is not None for path in psbt_parser.verified_input_derivation_paths) - - psbt_parser = PSBTParser(psbt, PSBTTestData.multisig_key_2, network=SettingsConstants.REGTEST) - assert any(path is not None for path in psbt_parser.verified_input_derivation_paths) - - psbt_parser = PSBTParser(psbt, PSBTTestData.multisig_key_3, network=SettingsConstants.REGTEST) - assert any(path is not None for path in psbt_parser.verified_input_derivation_paths) + # The fixture has one input; each cosigner's seed must verify on it + for seed in [PSBTTestData.seed, PSBTTestData.multisig_key_2, PSBTTestData.multisig_key_3]: + psbt_parser = PSBTParser(psbt, seed, network=SettingsConstants.REGTEST) + assert psbt_parser.verified_input_derivation_paths[0] != [] def test_a_psbt_with_no_utxos_is_rejected_rather_than_crashing(self): @@ -1424,7 +1444,7 @@ def test__parse__counts_a_multisig_output_paying_other_people_as_a_spend(self): # 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 + assert psbt_parser.verified_output_derivation_paths[0] == [] # The parser correctly categorized the output as an external spend assert psbt_parser.change_data == [] @@ -1463,7 +1483,7 @@ def test__parse__counts_multisig_change_with_no_derivation_paths_as_a_spend(self # 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 + assert psbt_parser.verified_output_derivation_paths[0] != [] # And the output was correctly categorized as change assert psbt_parser.change_amount == 10_000 @@ -1478,7 +1498,7 @@ def test__parse__counts_multisig_change_with_no_derivation_paths_as_a_spend(self # 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 + assert psbt_parser.verified_output_derivation_paths[0] == [] # Because we couldn't do proper verification, the parser correctly categorized the # output as an external spend. @@ -1505,7 +1525,7 @@ def test__parse__counts_multisig_change_with_no_script_as_a_spend(self): psbt_parser = self._parse(psbt) # The claim itself still verifies - assert psbt_parser.verified_output_derivation_paths[0] is not None + assert psbt_parser.verified_output_derivation_paths[0] != [] # But with no script there is no m-of-n to compare, so the output never becomes a # change candidate at all. @@ -1553,7 +1573,7 @@ def test__parse__counts_taproot_change_naming_our_internal_key_as_a_spend(self): 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 + assert psbt_parser.verified_output_derivation_paths[0] != [] # ...the parser can't fully verify the output as change, so has to report it as an # external spend. @@ -1597,7 +1617,7 @@ def test__parse__counts_taproot_change_naming_our_script_tree_key_as_a_spend(sel 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 + assert psbt_parser.verified_output_derivation_paths[0] != [] # ...but the output still has to be reported as an external spend assert psbt_parser.change_amount == 0 @@ -1784,33 +1804,29 @@ def test__parse__rejects_a_multisig_output_that_hides_this_seed_behind_another_f self._parse(psbt) - def test__parse__refuses_a_multisig_decoy_entry_in_either_position(self): + def test__parse__refuses_a_multisig_decoy_entry(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. + genuinely belongs to our seed, but a decoy derivation path entry is added as an + extra entry or as a replacement for another cosigner's. 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. + We don't need to decide if such a psbt has malicious intent; the decoy is a + contradiction 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. + this test covers: + * Decoy listed before the seed's actual cosigner entry. + * Decoy listed after it. + * Decoy substituted for another cosigner's entry (so the output still lists + exactly as many entries as its script has keys). - 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. + The presence of a decoy in any of the placements should raise + PSBTOutputOwnershipContradictionError. """ root = self._root() @@ -1820,39 +1836,79 @@ def test__parse__refuses_a_multisig_decoy_entry_in_either_position(self): (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 + psbt = self._psbt_with_change(input_base64, change_hex) + cosigner_entries = dict(psbt.outputs[0].bip32_derivations) - 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 + # 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) + + # Decoy listed first (note: dicts preserve insertion order) + decoy_first = {decoy_public_key: decoy_entry} + decoy_first.update(cosigner_entries) + + # Decoy listed last + decoy_last = dict(cosigner_entries) + decoy_last[decoy_public_key] = decoy_entry + + # Decoy in place of another cosigner's entry + decoy_substituted = dict(cosigner_entries) + for public_key, entry in cosigner_entries.items(): + if entry.fingerprint != root.my_fingerprint: + del decoy_substituted[public_key] + break + decoy_substituted[decoy_public_key] = decoy_entry + assert len(decoy_substituted) == len(cosigner_entries) + + # Run all three placements of the decoy + for entries in [decoy_first, decoy_last, decoy_substituted]: + psbt.outputs[0].bip32_derivations = entries + + # Prep the our modified psbt in embit + tampered_psbt = PSBT.parse(psbt.serialize()) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + PSBTParser(tampered_psbt, self.seed, network=SettingsConstants.REGTEST) + + + # def test__parse__accepts_a_multisig_output_holding_this_seed_in_two_slots(self): + # """ + # An edge case 2-of-3 that uses the same seed for two of its keys, each at its own + # derivation path. A legitimate change output for such a multisig should be + # recognized as change. + + # Test not built; the setup complexity for this test is more effort than it's + # worth for a wallet nobody would / should set up. + # """ + # pass + + + def test__parse__rejects_a_multisig_output_padded_with_a_strangers_entry(self): + """ + An honest multisig change output, plus one extra derivation path entry claiming a + stranger's fingerprint. Our own entry verifies and our key is in the script, so + the output's account of itself holds up as far as this seed can check. But the + script has only as many keys as it has cosigners, so the extra entry describes a + key the script never uses. The parser rejects the psbt with + PSBTSurplusDerivationPathsError. + """ + 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) + out = psbt.outputs[0] + assert len(out.bip32_derivations) == 3 + + claim_seed_owns_key(out, "m/48h/1h/0h/2h/1/0", foreign_public_key(), seed=PSBTTestData.recipient_multisig_key_2) + assert len(out.bip32_derivations) == 4 - with pytest.raises(expected_error): - self._parse(psbt) + with pytest.raises(PSBTSurplusDerivationPathsError): + PSBTParser(psbt, self.seed, network=SettingsConstants.REGTEST) def test__parse__rejects_a_multisig_output_whose_supplied_script_is_not_its_own(self): @@ -2025,7 +2081,7 @@ def test__parse__counts_a_different_quorum_as_a_spend(self): # 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 + assert psbt_parser.verified_output_derivation_paths[0] != [] # But the output pays a different quorum than the inputs spend from, so it # is counted as a spend. From 74cf69248e44455231210ad71e64aba6f3fb8fdd Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:57:08 -0500 Subject: [PATCH 3/6] Trivial comment typo fix --- tests/test_psbt_parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index ad82abfef..fbe6b16ff 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -1867,7 +1867,7 @@ def test__parse__refuses_a_multisig_decoy_entry(self): for entries in [decoy_first, decoy_last, decoy_substituted]: psbt.outputs[0].bip32_derivations = entries - # Prep the our modified psbt in embit + # Prep the modified psbt in embit tampered_psbt = PSBT.parse(psbt.serialize()) with pytest.raises(PSBTOutputOwnershipContradictionError): From 34fb17490fbc4fea3dcc137876517cc6736f90c1 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:03:35 -0500 Subject: [PATCH 4/6] Trivial style change: more explicit conditional check --- src/seedsigner/models/psbt_parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index fffed76e2..66d51d26d 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -1128,7 +1128,7 @@ def _reject_if_seed_cannot_sign(self): # cosigner, ours among them). One verified input path is enough for the psbt to # be signable. for verified_derivation_paths in self.verified_input_derivation_paths: - if verified_derivation_paths != []: + if len(verified_derivation_paths) > 0: return # There's nothing for this seed to sign From 81ae37a5c1e6b0430a56d44e3865af0b0564870f Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:23:48 -0500 Subject: [PATCH 5/6] Admit bare p2sh outputs as nested single sig change A p2sh-p2wpkh output's scriptPubKey is a bare p2sh hash. BIP-174 leaves the redeem script optional on an output, and without it _get_policy types the output as plain p2sh, which does not match a p2sh-p2wpkh wallet policy. The output then never reaches the ownership check at all. Two things follow. The user's own change is displayed as a payment out to a stranger. And an output that keeps a genuine claim on this seed while repointing its scriptPubKey escapes the ownership-contradiction refusal by dropping one optional field. Nested single sig is the only script type whose identity depends on an optional field its proof does not read: the rebuild is p2sh(p2wpkh(K)) from our own seed at the claimed path and the inputs' policy, so the missing field is a discriminator rather than evidence. For p2sh-p2wsh the discriminator and the proof material are the same field, so its absence is genuine silence and correctly out of reach. _policy_shape_matches becomes _is_change_candidate, an instance method, since it now reads the inputs' policy and the output's verified derivation paths. Beside the unchanged shape comparison it admits a bare p2sh output under a p2sh-p2wpkh wallet that carries no m-of-n, supplies exactly one derivation path entry, and holds one verified claim on this seed. Everything below is unchanged: the rebuild decides, honest change is counted as change, and a repointed output raises PSBTOutputOwnershipContradictionError. The single-entry conditions are what keep that refusal safe. A genuine multisig output carries one derivation path entry per cosigner, so it never reaches the contradiction check by this route. The shape that would is a bare p2sh output withholding both scripts while annotating a single key of a multisig, and no surveyed coordinator emits one. Every coordinator but Bitcoin Core annotates only its own change output. Core annotates an output because a descriptor solved its script, so it holds all n key origins and writes them. --- src/seedsigner/models/psbt_parser.py | 49 +++++++++++++++++++--------- tests/test_psbt_parser.py | 43 ++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 66d51d26d..e81781a24 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -240,7 +240,7 @@ def parse(self): _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 between inputs. - Outputs deliberately compare shape alone; see _policy_shape_matches. + Outputs deliberately compare shape alone; see _is_change_candidate. 5. _parse_outputs: organizes the output data (amounts, destination_addresses, etc.) and verifies the ownership of the outputs that come back to this seed @@ -399,7 +399,7 @@ def _parse_outputs(self, child_key_derivation_cache: dict): # 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): + if self._is_change_candidate(out, out_policy, self.verified_output_derivation_paths[i]): # Begin the extensive work to fully verify whether this output is indeed # change. @@ -696,23 +696,42 @@ def _get_policy(scope, scriptpubkey, xpubs, child_key_derivation_cache: dict | N return policy - @staticmethod - def _policy_shape_matches(policy_a: dict, policy_b: dict) -> bool: + def _is_change_candidate(self, out: OutputScope, out_policy: dict, verified_derivation_paths: List[DerivationPath]) -> 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. + Determines whether an output is worth the full ownership check in _parse_outputs. + + Returns True if the output's policy has the same "shape" as the inputs' policy: + the script type, plus m-of-n for multisig. + + One outlier: Nested single sig (p2sh-p2wpkh). Its scriptPubKey is a p2sh hash of + its redeem script, but per BIP-174 the redeem script itself is optional. + When it is omitted, the output is superficially indistinguishable from plain p2sh. + If the inputs are p2sh-p2wpkh, then such an output would fail the policy + comparison test (p2sh != p2sh-p2wpkh) when it may have actually been possible to + verify it as our change. + + So instead, when a p2sh output could be our own nested single sig change we let it + through and leave it to the rebuild process to verify if the output really is our + change. + + Note: A multisig's input or output policy can also include the cosigners if + they're supplied in the global xpubs. But this function does not take the + cosigners into account; cosigner information, if provided, is evaluated later. """ + # The outlier: a single sig p2sh output when the inputs are p2sh-p2wpkh. + if ( + self.policy["type"] == "p2sh-p2wpkh" # Input policy criteria + and out_policy["type"] == "p2sh" # Output policy criteria + and "m" not in out_policy # Exclude multisig + and len(out.bip32_derivations) == 1 # Nested single sig pays just one key + and len(verified_derivation_paths) == 1 # And that one key must be ours + ): + return True + + # The usual test: the output's policy has the same shape as the inputs' policy. for field in ("type", "m", "n"): - if policy_a.get(field) != policy_b.get(field): + if out_policy.get(field) != self.policy.get(field): return False - return True diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index fbe6b16ff..9c662368c 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -1946,6 +1946,49 @@ def test__parse__rejects_a_multisig_output_whose_supplied_script_is_not_its_own( self._parse(psbt) + def test__parse__counts_nested_single_sig_change_without_its_redeem_script_as_change(self): + """ + A legitimate nested single sig (p2sh-p2wpkh) change output can omit its redeem + script (BIP-174 makes it optional) while still claiming (via bip32_derivations) + that a key owned by our seed will receive the change. + + But the parser's proof of ownership check does not care about the missing redeem + script: the parser rebuilds p2sh(p2wpkh(K)) from the seed's own key at the claimed + path regardless. So the output should still be verifiable as change. + """ + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + + # The output claims a single key and our seed really does derive it there. + assert len(psbt.outputs[0].bip32_derivations) == 1 + public_key, derivation_path = list(psbt.outputs[0].bip32_derivations.items())[0] + assert PSBTParser.seed_owns_pubkey(self._root(), derivation_path.derivation, public_key, child_key_derivation_cache=None) is True + + psbt.outputs[0].redeem_script = None + + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 10_000 + assert psbt_parser.spend_amount == 0 + + + def test__parse__rejects_a_bare_p2sh_output_that_claims_this_seed_but_pays_someone_else(self): + """ + Variation on the prior test: the redeem script is still omitted but this time the + psbt repoints its output at a stranger's p2sh-p2wpkh. Crucially, the output keeps + its claim on this seed, making this an attempt at deception (if there was no claim + on the output, it would simply be a typical external spend output). + + When the parser rebuilds p2sh(p2wpkh(K)), the resulting scriptPubKey will not + match what the output commits to. The psbt should be refused with + PSBTOutputOwnershipContradictionError. + """ + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + psbt.outputs[0].redeem_script = None + psbt.outputs[0].script_pubkey = script.p2sh(script.p2wpkh(foreign_public_key())) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + 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 From eb3091c6d1b36b0c495b4b2340e6585a64e8af4f Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:22:20 -0500 Subject: [PATCH 6/6] Verify input scripts against the scriptPubKey they commit to BIP-174 asks a signer to hash an input's redeem or witness script against the scriptPubKey being spent. We read the wallet policy (m-of-n, cosigner keys) out of those scripts and never checked them, so a coordinator's own script decided which outputs we displayed as the user's change. _verify_input_scripts runs on every input before the policy is read, and each refusal has its own screen: - A missing script raises PSBTMissingInputScriptError, graded as a correctness problem. - A script that hashes to the wrong value raises PSBTInputScriptMismatchError, graded as an attack. It is the same contradiction between a supplied script and its scriptPubKey that PSBTOutputOwnershipContradictionError grades as an attack on outputs. - An extraneous script (a witness script on a legacy multisig input, a redeem script on a native segwit input) raises PSBTExtraneousInputScriptError, left ungraded. It is refused on every input type: there is no hash to check it against, and _get_policy decides a p2sh input is nested segwit from the mere presence of a witness script. Measured across the four p2sh and p2wsh fixtures: no signature over a wrong script was valid against the real coin, because embit builds the digest from the first supplied script (witness, then redeem, then scriptPubKey). Funds were never movable this way; what the check closes is the display. --- src/seedsigner/models/psbt_parser.py | 127 ++++++++++++++- src/seedsigner/views/psbt_views.py | 105 +++++++++++- tests/screenshot_generator/generator.py | 3 + tests/test_flows_psbt.py | 71 +++++++++ tests/test_psbt_parser.py | 203 +++++++++++++++++++++++- 5 files changed, 497 insertions(+), 12 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index e81781a24..7156b4447 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -97,12 +97,48 @@ class PSBTOutputOwnershipContradictionError(PSBTVerificationError): 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 PSBTMissingInputScriptError(PSBTVerificationError): + """ + An input is missing a required script. + + Required scripts: + * p2wsh: witness_script + * p2sh: redeem_script + * p2sh-p2wsh (nested segwit multisig): witness_script and redeem_script + + This is a correctness problem rather than an attack. + """ + pass + + +class PSBTInputScriptMismatchError(PSBTVerificationError): + """ + An input supplied a script that doesn't match what the input actually commits to. + + This sort of misleading script can lead the parser to an incorrect determination of + whether an output is our change. + + We treat this deception as an attack. + """ + pass + + +class PSBTExtraneousInputScriptError(PSBTVerificationError): + """ + An input commits to a specific script type but also supplies an extraneous script that + is not used by that script type (e.g. a redeem script is meaningless for native + segwit). + + We don't try to decide whether this is an attack or a mistake. + """ + pass + + class PSBTSeedCannotSignError(PSBTVerificationError): """ The selected seed holds no key that could sign any input. @@ -228,9 +264,13 @@ def parse(self): inputs can be signed by the seed. A mismatch rather than an attack, caught here so the flow can say so before showing a transaction. - 4. _parse_inputs: every input must resolve to the same policy otherwise a - RuntimeError is raised. TODO: make this a PSBTVerificationError subclass so - the view can deliberately catch this scenario and route accordingly. + 4. _parse_inputs: an input must supply every script it commits to and no others. + A missing script raises PSBTMissingInputScriptError, a wrong one + PSBTInputScriptMismatchError, and an extra one + PSBTExtraneousInputScriptError. Every input must then resolve to the same + policy otherwise a RuntimeError is raised (TODO: replace the RuntimeError + with a PSBTVerificationError subclass so the View can deliberately catch this + scenario and route accordingly). A policy is one of: - single-sig: the script type alone. Says nothing about keys. @@ -318,6 +358,10 @@ def _parse_inputs(self, child_key_derivation_cache: dict): self.input_amount += inp.utxo.value script_pubkey = inp.script_pubkey + # Verify the input's scripts and reject the psbt if verification fails + PSBTParser._verify_input_scripts(inp, script_pubkey) + + # Now we can safely use those scripts to determine this input's wallet policy inp_policy = PSBTParser._get_policy(inp, script_pubkey, self.psbt.xpubs, child_key_derivation_cache) if self.policy == None: self.policy = inp_policy @@ -326,6 +370,81 @@ def _parse_inputs(self, child_key_derivation_cache: dict): raise RuntimeError("Mixed inputs in the transaction") + @staticmethod + def _verify_input_scripts(inp: InputScope, script_pubkey: script.Script): + """ + Checks that an input supplies exactly the scripts it commits to. + + A p2sh or p2wsh scriptPubKey holds only a hash of the script the input spends + with. Per BIP-174, the psbt must supply that script. Missing scripts raise + PSBTMissingInputScriptError. + + We then rebuild what the input commits to and compare, as BIP-174 requires of a + signer: + p2wsh: p2wsh(witness_script) == scriptPubKey + p2sh: p2sh(redeem_script) == scriptPubKey + p2sh-p2wpkh: p2sh(redeem_script) == scriptPubKey + + Multisig nested segwit has TWO layers to check: + p2sh-p2wsh: p2sh(redeem_script) == scriptPubKey + p2wsh(witness_script) == redeem_script + + If any comparison required above fails, we raise PSBTInputScriptMismatchError. + + Each input commits to a specific script type. It should not include any extraneous + scripts that are not required by that script type (e.g. a witness_script on a p2sh + input). Such a script could distort the parser's understanding of the wallet being + spent from, which would affect its determination of whether an output is the + user's change. But it's also possible that a buggy coordinator just created an odd + psbt. Either way, we raise PSBTExtraneousInputScriptError for any extraneous + script. + """ + script_type = script_pubkey.script_type() + expects_redeem_script = False + expects_witness_script = False + + if script_type == "p2wsh": + expects_witness_script = True + + if inp.witness_script is None: + raise PSBTMissingInputScriptError("Input commits to a witness script it did not supply") + + # The scriptPubKey holds a hash of the witness script, so we rebuild the + # scriptPubKey from the supplied one and compare. + if script.p2wsh(inp.witness_script).data != script_pubkey.data: + raise PSBTInputScriptMismatchError("Input's witness script is not the one its scriptPubKey commits to") + + elif script_type == "p2sh": + expects_redeem_script = True + + if inp.redeem_script is None: + raise PSBTMissingInputScriptError("Input commits to a redeem script it did not supply") + + # The scriptPubKey holds a hash of the redeem script, so we rebuild the + # scriptPubKey from the supplied one and compare. For legacy p2sh multisig and + # p2sh-p2wpkh, the redeem script is the only layer. + if script.p2sh(inp.redeem_script).data != script_pubkey.data: + raise PSBTInputScriptMismatchError("Input's redeem script is not the one its scriptPubKey commits to") + + # Nested segwit: the redeem script is itself a commitment to a witness script, + # so p2sh-p2wsh has a second layer to check. + if inp.redeem_script.script_type() == "p2wsh": + expects_witness_script = True + + if inp.witness_script is None: + raise PSBTMissingInputScriptError("Nested segwit input commits to a witness script it did not supply") + + if script.p2wsh(inp.witness_script).data != inp.redeem_script.data: + raise PSBTInputScriptMismatchError("Nested segwit input's witness script is not the one its redeem script commits to") + + # Any script beyond the ones this input commits to is extraneous + if inp.redeem_script is not None and not expects_redeem_script: + raise PSBTExtraneousInputScriptError("Input supplied a redeem script its scriptPubKey does not commit to") + + if inp.witness_script is not None and not expects_witness_script: + raise PSBTExtraneousInputScriptError("Input supplied a witness script its scriptPubKey does not commit to") + + def _parse_outputs(self, child_key_derivation_cache: dict): """ Sorts each output into change coming back to this seed, an external spend, or diff --git a/src/seedsigner/views/psbt_views.py b/src/seedsigner/views/psbt_views.py index dba57fb43..79eae72e6 100644 --- a/src/seedsigner/views/psbt_views.py +++ b/src/seedsigner/views/psbt_views.py @@ -1,9 +1,10 @@ from gettext import gettext as _ -from seedsigner.models.psbt_parser import (PSBTInputOwnershipClaimError, - PSBTMixedDerivationPathTypesError, PSBTOutputOwnershipClaimError, - PSBTOutputOwnershipContradictionError, PSBTParser, PSBTSeedCannotSignError, - PSBTSurplusDerivationPathsError) +from seedsigner.models.psbt_parser import (PSBTExtraneousInputScriptError, + PSBTInputOwnershipClaimError, PSBTInputScriptMismatchError, + PSBTMissingInputScriptError, 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) @@ -123,6 +124,18 @@ def __init__(self): self.set_redirect(Destination(PSBTMixedDerivationPathTypesView, clear_history=True)) return + except PSBTMissingInputScriptError: + self.set_redirect(Destination(PSBTMissingInputScriptView, clear_history=True)) + return + + except PSBTInputScriptMismatchError: + self.set_redirect(Destination(PSBTInputScriptMismatchView, clear_history=True)) + return + + except PSBTExtraneousInputScriptError: + self.set_redirect(Destination(PSBTExtraneousInputScriptView, clear_history=True)) + return + except PSBTOutputOwnershipContradictionError: self.set_redirect(Destination(PSBTOutputOwnershipContradictionView, clear_history=True)) return @@ -614,6 +627,90 @@ def run(self): +class PSBTMissingInputScriptView(View): + """ + Reached when an input commits to a script that the psbt did not supply (see + PSBTMissingInputScriptError). + + We view this as a correctness problem rather than an attack. 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 left out a script that one of its inputs needs."), + 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 PSBTInputScriptMismatchView(View): + """ + Reached when an input supplies a script that hashes to something other than what the + input commits to (see PSBTInputScriptMismatchError). + + 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 supplied the wrong script for one of its inputs."), + 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 PSBTExtraneousInputScriptView(View): + """ + Reached when an input supplies a script beyond the ones its scriptPubKey commits to + (see PSBTExtraneousInputScriptError). + + We do not try to decide whether this is an attack or a mistake. 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 supplied an extra script for one of its inputs."), + 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 PSBTAddressVerificationFailedView(View): """ Reached from PSBTChangeDetailsView when a multisig change or self-transfer output diff --git a/tests/screenshot_generator/generator.py b/tests/screenshot_generator/generator.py index 5fa52a2cc..22f6c57a8 100644 --- a/tests/screenshot_generator/generator.py +++ b/tests/screenshot_generator/generator.py @@ -452,6 +452,9 @@ def mock_version_to_most_recent_release(): ScreenshotConfig(psbt_views.PSBTSurplusDerivationPathsView), ScreenshotConfig(psbt_views.PSBTMixedDerivationPathTypesView), ScreenshotConfig(psbt_views.PSBTOutputOwnershipContradictionView), + ScreenshotConfig(psbt_views.PSBTMissingInputScriptView), + ScreenshotConfig(psbt_views.PSBTInputScriptMismatchView), + ScreenshotConfig(psbt_views.PSBTExtraneousInputScriptView), 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), diff --git a/tests/test_flows_psbt.py b/tests/test_flows_psbt.py index c21a3e050..4782f3ad1 100644 --- a/tests/test_flows_psbt.py +++ b/tests/test_flows_psbt.py @@ -320,6 +320,77 @@ def test_mixed_derivation_path_types_terminate_signing_flow(self): ]) + def test_missing_input_script_terminates_signing_flow(self): + """ + The psbt leaves out the witness script that a p2wsh input commits to. + + The flow should stop at PSBTMissingInputScriptView, before any transaction detail + is rendered. + """ + psbt = PSBT.parse(a2b_base64(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT)) + psbt.outputs.append(create_output(PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE, 10_000)) + psbt.inputs[0].witness_script = None + + self._load_psbt_for_signing(psbt) + + self.run_sequence([ + FlowStep(psbt_views.PSBTSelectSeedView, screen_return_value=0), + FlowStep(psbt_views.PSBTOverviewView, is_redirect=True), + FlowStep(psbt_views.PSBTMissingInputScriptView, button_data_selection=psbt_views.PSBTMissingInputScriptView.DISCARD), + FlowStep(MainMenuView), + ]) + + + def test_input_script_mismatch_terminates_signing_flow(self): + """ + The psbt replaces a p2wsh input's witness script with a stranger's. The coin + really is the user's, but the supplied script hashes to something other than what + the input's scriptPubKey commits to. + + The flow should stop at PSBTInputScriptMismatchView, before any transaction detail + is rendered. + """ + psbt = PSBT.parse(a2b_base64(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT)) + psbt.outputs.append(create_output(PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE, 10_000)) + foreign_multisig_keys = [foreign_public_key(f"m/48h/1h/0h/2h/0/{i}") for i in range(3)] + psbt.inputs[0].witness_script = script.multisig(2, foreign_multisig_keys) + + self._load_psbt_for_signing(psbt) + + self.run_sequence([ + FlowStep(psbt_views.PSBTSelectSeedView, screen_return_value=0), + FlowStep(psbt_views.PSBTOverviewView, is_redirect=True), + FlowStep(psbt_views.PSBTInputScriptMismatchView, button_data_selection=psbt_views.PSBTInputScriptMismatchView.DISCARD), + FlowStep(MainMenuView), + ]) + + + def test_extraneous_input_script_terminates_signing_flow(self): + """ + The psbt adds a witness script to a legacy multisig input, in addition to the + input's own redeem script. The coin really is the user's and the redeem script + still hashes to the scriptPubKey, but a legacy p2sh multisig input should never + have a witness script. + + The flow should stop at PSBTExtraneousInputScriptView, before any transaction + detail is rendered. + """ + psbt = PSBT.parse(a2b_base64(PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT)) + psbt.outputs.append(create_output(PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE, 10_000)) + + # The extraneous script's content is irrelevant, so an arbitrary script will do + psbt.inputs[0].witness_script = script.Script(b"\x51") # OP_TRUE + + self._load_psbt_for_signing(psbt) + + self.run_sequence([ + FlowStep(psbt_views.PSBTSelectSeedView, screen_return_value=0), + FlowStep(psbt_views.PSBTOverviewView, is_redirect=True), + FlowStep(psbt_views.PSBTExtraneousInputScriptView, button_data_selection=psbt_views.PSBTExtraneousInputScriptView.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 9c662368c..9c141f9c3 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -10,10 +10,11 @@ from embit.psbt import PSBT, DerivationPath, OutputScope from embit.descriptor import Descriptor -from seedsigner.models.psbt_parser import (PSBTInputOwnershipClaimError, - PSBTMixedDerivationPathTypesError, PSBTOutputOwnershipClaimError, - PSBTOutputOwnershipContradictionError, PSBTParser, PSBTSeedCannotSignError, - PSBTSurplusDerivationPathsError) +from seedsigner.models.psbt_parser import (PSBTExtraneousInputScriptError, + PSBTInputOwnershipClaimError, PSBTInputScriptMismatchError, + PSBTMissingInputScriptError, PSBTMixedDerivationPathTypesError, + PSBTOutputOwnershipClaimError, PSBTOutputOwnershipContradictionError, PSBTParser, + PSBTSeedCannotSignError, PSBTSurplusDerivationPathsError) from seedsigner.models.seed import Seed from seedsigner.models.settings_definition import SettingsConstants @@ -2252,3 +2253,197 @@ def p2pk(public_key: PublicKey) -> script.Script: with pytest.raises(RuntimeError, match="Unsupported policy type"): self._parse(psbt) + + +class TestPSBTParserInputScripts(PSBTParserOwnershipTestBase): + """ + Tests that an input supplies exactly the scripts it commits to. + + A p2sh or p2wsh input's scriptPubKey holds only a hash of the script it spends with, + so the psbt has to supply that script for anything to be checked. These tests cover + the parser requiring that script, requiring it to be the right one, and refusing an + extraneous one. + """ + 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 _add_foreign_nested_segwit_input(self, psbt: PSBT, derivation_path: str = "m/49h/1h/0h/0/0"): + """ + Adds another party's p2sh-p2wpkh input with no derivation paths, as in a payjoin: + their utxo, their key, and a redeem script built from their key. + + The supplied psbt's first input should be a witness_utxo input. + """ + # Start from a copy of our own input for its utxo fields + foreign_input = deepcopy(psbt.inputs[0]) + + # Strip the derivation paths so the input claims none of the user's keys + foreign_input.bip32_derivations.clear() + + # Give it the other party's key: a redeem script built from that key and a + # scriptPubKey that commits to that redeem script. + public_key = foreign_public_key(derivation_path) + foreign_input.redeem_script = script.p2wpkh(public_key) + foreign_input.witness_utxo.script_pubkey = script.p2sh(foreign_input.redeem_script) + + psbt.inputs.append(foreign_input) + return foreign_input + + + def test__parse__refuses_an_input_that_omits_the_script_it_commits_to(self): + """ + The psbt leaves out a script that an input commits to: + * p2wsh: witness_script + * legacy p2sh multisig: redeem_script + * p2sh-p2wpkh: redeem_script + * p2sh-p2wsh: witness_script or redeem_script + + Each case should raise PSBTMissingInputScriptError. + """ + # Each case first confirms that the fixture really does carry the script it is + # about to omit. + + # p2wsh: omit the witness script + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE) + assert psbt.inputs[0].witness_script is not None + psbt.inputs[0].witness_script = None + with pytest.raises(PSBTMissingInputScriptError): + self._parse(psbt) + + # legacy p2sh multisig: omit the redeem script + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE) + assert psbt.inputs[0].redeem_script is not None + psbt.inputs[0].redeem_script = None + with pytest.raises(PSBTMissingInputScriptError): + self._parse(psbt) + + # p2sh-p2wpkh: omit the redeem script + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + assert psbt.inputs[0].redeem_script is not None + psbt.inputs[0].redeem_script = None + with pytest.raises(PSBTMissingInputScriptError): + self._parse(psbt) + + # p2sh-p2wsh: omit the witness script + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE) + assert psbt.inputs[0].witness_script is not None + psbt.inputs[0].witness_script = None + with pytest.raises(PSBTMissingInputScriptError): + self._parse(psbt) + + # p2sh-p2wsh: omit the redeem script + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE) + assert psbt.inputs[0].redeem_script is not None + psbt.inputs[0].redeem_script = None + with pytest.raises(PSBTMissingInputScriptError): + self._parse(psbt) + + + def test__parse__refuses_an_input_that_supplies_the_wrong_script(self): + """ + The psbt replaces one of an input's own scripts with a stranger's. A script that + hashes to the wrong value should raise PSBTInputScriptMismatchError, at each layer + of each script type that has one. + """ + foreign_script = self._foreign_multisig_script() + + # p2wsh: the witness script is the only layer + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE) + psbt.inputs[0].witness_script = foreign_script + with pytest.raises(PSBTInputScriptMismatchError): + self._parse(psbt) + + # legacy p2sh multisig: the redeem script is the only layer + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE) + psbt.inputs[0].redeem_script = foreign_script + with pytest.raises(PSBTInputScriptMismatchError): + self._parse(psbt) + + # p2sh-p2wpkh: a redeem script built from someone else's key + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + psbt.inputs[0].redeem_script = script.p2wpkh(foreign_public_key()) + with pytest.raises(PSBTInputScriptMismatchError): + self._parse(psbt) + + # p2sh-p2wsh, inner layer: the genuine redeem script, but a witness script other + # than the one it commits to + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE) + psbt.inputs[0].witness_script = foreign_script + with pytest.raises(PSBTInputScriptMismatchError): + self._parse(psbt) + + # p2sh-p2wsh, outer layer: a redeem script other than the one the scriptPubKey + # commits to + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE) + psbt.inputs[0].redeem_script = script.p2wsh(foreign_script) + with pytest.raises(PSBTInputScriptMismatchError): + self._parse(psbt) + + + def test__parse__refuses_an_input_that_supplies_an_extra_script(self): + """ + The psbt adds an extraneous script that is nonsensical to include for the input's + script type: + * legacy p2sh multisig: should never have a witness_script + * p2wpkh: should never have a redeem_script + * p2sh-p2wpkh: should never have a witness_script + + The input's required script(s) must pass validation in order to reach the + extraneous script check, so those required scripts are preserved in their correct + form here. + + By rule, an extraneous script should raise PSBTExtraneousInputScriptError. + """ + # The extraneous script's content is irrelevant, so an arbitrary script will do + arbitrary_script = script.Script(b"\x51") # OP_TRUE + + # legacy p2sh multisig: add a witness script + psbt = self._psbt_with_change(PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE) + psbt.inputs[0].witness_script = arbitrary_script + with pytest.raises(PSBTExtraneousInputScriptError): + self._parse(psbt) + + # p2wpkh: add a redeem script + psbt = self._psbt_with_change() + psbt.inputs[0].redeem_script = arbitrary_script + with pytest.raises(PSBTExtraneousInputScriptError): + self._parse(psbt) + + # p2sh-p2wpkh: add a witness script + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + psbt.inputs[0].witness_script = arbitrary_script + with pytest.raises(PSBTExtraneousInputScriptError): + self._parse(psbt) + + + def test__parse__checks_input_scripts_whoever_the_input_belongs_to(self): + """ + A collaborative spend, such as a payjoin, puts another party's input alongside the + user's. That input's scripts should be checked just as the user's are. Whose + input it is rests on the psbt's own claims, so no input is exempt from the script + checks. + + The psbt will be rejected if a script is missing (PSBTMissingInputScriptError) or + wrong (PSBTInputScriptMismatchError). + """ + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + foreign_input = self._add_foreign_nested_segwit_input(psbt) + + # The other party's input carries its correct redeem script and none of the + # user's keys. + psbt_parser = self._parse(psbt) + assert psbt_parser.num_inputs == 2 + assert psbt_parser.verified_input_derivation_paths[1] == [] + + # Omit its redeem script + foreign_input.redeem_script = None + with pytest.raises(PSBTMissingInputScriptError): + self._parse(psbt) + + # With a redeem script built from the other party's key at the next address + # (index 1), rather than the index 0 key that the scriptPubKey commits to. + foreign_input.redeem_script = script.p2wpkh(foreign_public_key("m/49h/1h/0h/0/1")) + with pytest.raises(PSBTInputScriptMismatchError): + self._parse(psbt)