Narrow bare p2sh candidacy to what the rebuild cannot decide
What changed, and why it matters
This commit fixes a logic flaw in how SeedSigner decides whether a Bitcoin transaction output is 'change' going back to the user's own wallet, versus a payment to someone else. Previously, a specially crafted PSBT could make an output that actually pays to the user's own key appear as a spend to an external address, or could make a contradictory ownership claim slip through. The patch narrows the special-case rule and adds tests to catch these scenarios.
Review and merge the patch, then verify that the new unit tests cover the three stated conditions and that no other change-detection shortcuts rely on fingerprint rather than verified key material.
Security signals we found
PSBT output ownership misclassification
Fingerprint vs key derivation mismatch in change detection
Contradiction between supplied derivation path and redeem script omission
Defensive hardening of change-output heuristics
Evidence from the diff
The change is in PSBT output classification. The old _is_change_candidate special case for bare p2sh change from p2sh-p2wpkh inputs required the output’s single BIP32 derivation entry to belong to this seed. That was too strict and also let through a case where the PSBT lists another seed’s fingerprint at the same path while the scriptPubKey actually pays this seed’s key. The rebuild derives the key from the path regardless of fingerprint, so the output matched and was wrongly treated as change. The patch replaces the fingerprint/derivation-path ownership checks for this outlier with a simpler condition: inputs are p2sh-p2wpkh, output parses as plain p2sh, and the redeem script is omitted. This aligns the candidate test with what the rebuild can and cannot verify. New tests enforce that a bare p2sh output paying our key but claiming another key is refused as a contradiction, and that payments to other wallets of this seed are counted as spends.
Changed components
src/seedsigner/models/psbt_parser.pytests/test_psbt_parser.pyInspect captured patch +70 / −8
### src/seedsigner/models/psbt_parser.py
@@ -411,7 +411,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 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 @@ def _get_policy(scope, scriptpubkey, xpubs, child_key_derivation_cache: dict | N
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 @@ def _is_change_candidate(self, out: OutputScope, out_policy: dict, verified_deri
"""
# 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
### tests/test_psbt_parser.py
@@ -2105,6 +2105,70 @@ def test__parse__rejects_a_bare_p2sh_output_that_claims_this_seed_but_pays_someo
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 theWhy this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.