From cf18a0b9f2181afae9b7e2bc50aa394be8c42049 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 1/3] 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 | 47 +++++++++++++++++++--------- tests/test_psbt_parser.py | 43 +++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 14 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 2f62066d..7c51f94a 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -249,7 +249,7 @@ class PSBTParser(): _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 @@ -411,7 +411,7 @@ class PSBTParser(): # 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. @@ -708,23 +708,42 @@ class PSBTParser(): 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. + Determines whether an output is worth the full ownership check in _parse_outputs. - 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. + 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 588dc393..40b6ce24 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -2062,6 +2062,49 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): 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 f04675e4a3d8241a5c70a98123c4c322ca706a9f Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:02:11 -0500 Subject: [PATCH 2/3] Narrow bare p2sh candidacy to what the rebuild cannot decide The nested single sig outlier in change candidacy also required the output's one derivation path entry to claim this seed's fingerprint. That kept an output paying our key under another seed's key and fingerprint out of the rebuild, so it was shown as a spend to the user's own address, while the same output with its redeem script present is refused as a contradiction. The rebuild derives our key at the entry's path whatever fingerprint it lists, so the outlier drops the condition and _is_change_candidate drops the verified derivation paths it read for it. The outlier also required exactly one derivation path entry. It only kept out bare p2sh outputs annotated with several keys while omitting their scripts, which no surveyed coordinator emits, and the single sig surplus check now refuses those. The m-of-n exclusion becomes a check that the output omits its redeem script. For an output parsed as plain p2sh the two admit the same outputs, since a supplied multisig redeem script is the only source of an m-of-n there, and the new check states what the outlier is for. New tests refuse an output that pays this seed but lists another key, and pin each of the three remaining conditions: a payment to another wallet of this seed, annotated with our key, stays a spend. --- src/seedsigner/models/psbt_parser.py | 14 +++--- tests/test_psbt_parser.py | 64 ++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 7c51f94a..16741a3d 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -411,7 +411,7 @@ class PSBTParser(): # 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 self._is_change_candidate(out, out_policy, self.verified_output_derivation_paths[i]): + if self._is_change_candidate(out, out_policy): # Begin the extensive work to fully verify whether this output is indeed # change. @@ -708,7 +708,7 @@ class PSBTParser(): return policy - def _is_change_candidate(self, out: OutputScope, out_policy: dict, verified_derivation_paths: List[DerivationPath]) -> bool: + def _is_change_candidate(self, out: OutputScope, out_policy: dict) -> bool: """ Determines whether an output is worth the full ownership check in _parse_outputs. @@ -732,15 +732,13 @@ class PSBTParser(): """ # 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 + self.policy["type"] == "p2sh-p2wpkh" # Input is nested single sig + and out_policy["type"] == "p2sh" # Output parses as plain p2sh + and out.redeem_script is None # Output omits its redeem script ): return True - # The usual test: the output's policy has the same shape as the inputs' policy. + # All other outputs must have the same policy shape as the inputs for field in ("type", "m", "n"): if out_policy.get(field) != self.policy.get(field): return False diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index 40b6ce24..4bb9103a 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -2105,6 +2105,70 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): self._parse(psbt) + def test__parse__rejects_a_bare_p2sh_output_that_pays_this_seed_but_lists_another_key(self): + """ + Another variation: the redeem script is omitted and the output still pays our key, + but its one derivation path entry lists another seed's key and fingerprint at the + same path. + + The parser rebuilds p2sh(p2wpkh(K)) from our own key at that path and ignores the + fingerprint the entry lists. The result based on our key matches what the output + commits to, so it actually is our output even though the psbt claimed that it paid + to a different key. 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 + + # Swap the output's entry for another seed's key at the same path. The + # scriptPubKey still pays our key there. + derivation_path = list(psbt.outputs[0].bip32_derivations.values())[0] + path = bip32.path_to_str(derivation_path.derivation) + psbt.outputs[0].bip32_derivations.clear() + claim_seed_owns_key(psbt.outputs[0], path, foreign_public_key(path), seed=PSBTTestData.recipient_seed) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + + def test__parse__counts_a_payment_to_another_wallet_of_this_seed_as_a_spend(self): + """ + A psbt can pay another wallet of this seed, such as its native segwit account or a + multisig it belongs to, and annotate that output with our key. Each case below is + such a payment. Each should parse and be counted as a spend. + + The parser makes an exception for nested single sig change that omits its redeem + script. An output must meet three criteria to qualify for the exception: + * inputs: the inputs are nested single sig + * output type: the output is parsed as plain p2sh + * redeem script: the output omits its redeem script + + These outputs should be categorized as external spends if any of the three + criteria are not met. Each scenario in this test sets up one criterion to fail + while the other two are met. + """ + # Fails the inputs condition: a native segwit input instead of nested single sig + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + psbt.outputs[0].redeem_script = None + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + # Fails the output type condition: p2wpkh is not parsed as a plain p2sh output + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_CHANGE) + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + # Fails the redeem script condition: legacy multisig is parsed as plain p2sh even + # when it supplies its redeem script. + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE) + assert psbt.outputs[0].redeem_script is not None + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + 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 f5cbc18a3c43bebea78aac5921e23686aef422c8 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:21:32 -0400 Subject: [PATCH 3/3] Restore why change candidacy ignores cosigners Renaming _policy_shape_matches to _is_change_candidate dropped the reason the candidacy test compares shape alone. Without it, adding the cosigners to the comparison reads as a harmless tightening. The old wording named a misannotated fingerprint as what breaks an output's cosigner resolution. Since #1032, _get_cosigners matches each key to a global xpub by derivation path alone and never reads the fingerprint, so the restored note names a misannotated derivation path instead. --- src/seedsigner/models/psbt_parser.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 16741a3d..501c23ca 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -728,7 +728,16 @@ class PSBTParser(): 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. + cosigners into account; comparing the cosigners here would let a psbt decide which + of its own outputs get verified: + * One misannotated derivation path would make that output's cosigners fail + to resolve. + * The output's missing cosigners would mean that it would not match the inputs' + cosigners. + * End result: the output would not be considered a change candidate and would + not go through the same scrutiny that change candidates do. + + Cosigner information, if provided, is evaluated later. """ # The outlier: a single sig p2sh output when the inputs are p2sh-p2wpkh. if (