laantungir/seedsigner
1
0
Fork 0
mirror of https://github.com/SeedSigner/seedsigner.git synced 2026-10-05 15:08:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

Admit bare p2sh outputs as nested single sig change

Browse Source 
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.
This commit is contained in:
kdmukai
2026-09-29 12:05:35 -05:00
parent cfaf443a5f
commit cf18a0b9f2
2 changed files with 76 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/seedsigner/models/psbt_parser.py
+33 -14
View File
@@ -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
tests/test_psbt_parser.py
+43
View File
@@ -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