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] 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