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