Count an unresolved output cosigner set as a mismatch
What changed, and why it matters
This commit fixes a bug in SeedSigner's PSBT parser that could misclassify a payment to a different multisig wallet as 'change' (money going back to the user's own wallet). The bug occurred when the output's cosigner list could not be resolved, because the comparison was skipped entirely. The fix now counts an unresolved or mismatched cosigner set as a spend instead of change. It also changes how keys are matched to xpubs: instead of first requiring the coordinator-provided fingerprint to match, it now trusts a matching derivation path and a proven derived key. This prevents a mislabeled fingerprint from turning a real change output into what looks like a spend.
Review and merge this defensive fix. Verify that the new matching logic (derivation path only, no fingerprint pre-check) does not weaken security in any scenario where a coordinator could supply a colliding derivation path. Consider whether additional tests are needed for single-sig or edge-case fingerprint behavior.
Security signals we found
Change-vs-spend misclassification in multisig PSBT parsing
Cosigner resolution failure previously skipped instead of treated as mismatch
Fingerprint matching removed in favor of derivation-path + derived-key verification
Defensive hardening against buggy or maliciously edited PSBTs
Test coverage expanded for different-quorum scenarios
Evidence from the diff
In psbt_parser.py, the output cosigner comparison previously required both input_cosigners and output_cosigners to be non-None. If output cosigners failed to resolve (None), the mismatch check was skipped and the output could be treated as presumed change. The patch removes the output_cosigners is not None requirement, so any unresolved or differing output cosigner set now marks is_presumed_change = False. Additionally, _get_cosigners no longer requires origin_der.fingerprint == der.fingerprint before comparing derivation paths and derived keys; it now matches xpubs by derivation path alone and verifies the xpub actually derives the pubkey. Tests were updated to reflect realistic coordinator behavior (global xpubs only include the spending wallet’s keys) and to add a second test for the four-xpub variant.
Changed components
src/seedsigner/models/psbt_parser.pytests/test_psbt_parser.pyInspect captured patch +102 / −46
### src/seedsigner/models/psbt_parser.py
@@ -560,16 +560,14 @@ def _parse_outputs(self, child_key_derivation_cache: dict):
is_presumed_change = True
# One thing we can rule out now: if the psbt supplied global
- # xpubs (see _get_cosigners) AND it fully annotated this
- # output, we can see if this output's cosigners differ from
- # the inputs' cosigners. If so, then we can be sure this
- # output is NOT our change. This sort of mismatch is a
- # scenario that no known coordinator would produce, but
- # there's no harm in checking this edge case.
+ # xpubs (see _get_cosigners), we can compare this output's
+ # cosigners to the inputs' cosigners. Real change should have
+ # the same cosigners; if this output's cosigners differ or
+ # fail to resolve at all, we classify this output as NOT
+ # change.
input_cosigners = self.policy.get("cosigners")
output_cosigners = out_policy.get("cosigners")
- cosigners_resolved = input_cosigners is not None and output_cosigners is not None
- if cosigners_resolved and input_cosigners != output_cosigners:
+ if input_cosigners is not None and input_cosigners != output_cosigners:
is_presumed_change = False
elif verified_derivation_path is not None and self.policy["type"] != "p2tr":
@@ -846,8 +844,9 @@ def _get_cosigners(pubkeys, derivations, xpubs, child_key_derivation_cache: dict
fingerprint and derivation path, but only down to the account level (e.g.
m/48'/0'/0'/2'). A dict keyed on each xpub.
- The derivations and xpubs are unproven claims provided by the coordinator. So for
- each pubkey we check whether the xpub the psbt points us to really derives it.
+ The derivations and xpubs are unproven claims provided by the coordinator. So we
+ take each pubkey's claimed derivation path and check whether one of the xpubs
+ really derives that pubkey.
The resulting cosigners list consists of each xpub that provably derives each of
the script's keys. But that is ALL it proves. We have no way to verify who those
@@ -876,21 +875,21 @@ def _get_cosigners(pubkeys, derivations, xpubs, child_key_derivation_cache: dict
raise ValueError("Missing derivation")
der = derivations[pubkey]
- # Scan the xpubs for one whose fingerprint and derivation path match the
- # claim (xpub path comparisons have to stop at the account level).
+ # Scan the xpubs for one whose derivation path matches the claim.
for xpub in xpubs:
origin_der = xpubs[xpub]
- if origin_der.fingerprint == der.fingerprint:
- if origin_der.derivation == der.derivation[:-2]:
- # Then derive the actual child key (its full derivation path is
- # two indices deeper than the xpub's stated derivation path)
- derived_key = PSBTParser._derive_with_cache(xpub, der.derivation[-2:], child_key_derivation_cache)
-
- # Finally, compare that key with the target pubkey
- if derived_key.key == pubkey:
- # append strings so they can be sorted and compared
- cosigners.append(xpub.to_base58())
- break
+ # The full derivation path goes two indices deeper than the xpub's so we
+ # omit those last two when comparing.
+ if origin_der.derivation == der.derivation[:-2]:
+ # Derive the child key that sits two indices below the xpub (i.e. at
+ # the full derivation path).
+ derived_key = PSBTParser._derive_with_cache(xpub, der.derivation[-2:], child_key_derivation_cache)
+
+ # Finally, compare that key with the target pubkey
+ if derived_key.key == pubkey:
+ # Append as strings so they can be sorted and compared
+ cosigners.append(xpub.to_base58())
+ break
# Every key in the script has to trace back to an xpub for the result to mean
# anything.
### tests/test_psbt_parser.py
@@ -1919,6 +1919,10 @@ def _repoint_at_a_different_quorum(self, psbt: PSBT):
still holds this seed's key, but with one cosigner swapped for a different seed.
That swap makes the output's 2-of-3 a different wallet from the one the inputs
spend from.
+
+ The psbt's global xpubs are left as the fixture wrote them: the inputs' three
+ cosigners (the expected behavior for all known coordinators). The outsider's xpub
+ is not added to the global xpubs.
"""
# Callers pass a psbt whose change output comes first
out = psbt.outputs[0]
@@ -1943,10 +1947,6 @@ def _repoint_at_a_different_quorum(self, psbt: PSBT):
outsider_account = outsider_root.derive(account_derivation_path)
outsider_public_key = outsider_account.derive(address_derivation_path[-2:]).get_public_key()
- # Add the outsider's account xpub to the psbt's global xpubs, which now hold
- # four: the inputs' three cosigners plus the outsider.
- psbt.xpubs[outsider_account.to_public()] = DerivationPath(outsider_root.my_fingerprint, account_derivation_path)
-
# Rebuild the multisig script with the outsider's key in the displaced key's
# slot.
original_script = out.witness_script if out.witness_script is not None else out.redeem_script
@@ -1988,16 +1988,23 @@ def test__parse__counts_a_different_quorum_as_a_spend(self):
Every ownership check passes: the output commits to a script holding this seed's
key and the psbt claims this seed there truthfully.
- The psbt supplies global xpubs (the three from the input plus the outsider's that
- is part of the output) and it fully annotates this output's derivation paths,
- allowing for the parser to determine the output's cosigners accurately.
+ The psbt supplies the global xpubs of the wallet the inputs spend from. The output
+ is fully annotated so the parser tries to trace each of the output's three keys
+ back to one of those xpubs. The two keys it shares with the inputs resolve, but no
+ xpub derives the outsider's key. So the output is counted as a spend.
- End result of this setup: The output's cosigner list differs from the inputs'
- list. So the output is counted as a spend.
+ tldr: different output quorum + global xpubs + annotated external output
- This sort of cosigner mismatch is a scenario that no known coordinator would
- produce; normally an output to a different wallet would not be annotated nor have
- its xpubs added to the global xpubs data.
+ No known coordinator provides the global xpubs AND annotates an output paying a
+ different wallet.
+ * Every coordinator that writes global xpubs: annotates only its own wallet's
+ outputs.
+ * Bitcoin Core: annotates an output paying any wallet in its wallet file, but
+ writes no global xpubs.
+ * Note: a coordinator may exclude both global xpubs and all output annotations.
+ Such coordinators are irrelevant for this test.
+
+ So the check exists to catch buggy software or a maliciously edited psbt.
"""
for input_base64, change_hex in [
(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE),
@@ -2026,27 +2033,77 @@ def test__parse__counts_a_different_quorum_as_a_spend(self):
assert psbt_parser.spend_amount == 10_000
+ def test__parse__counts_a_different_quorum_as_a_spend_when_its_xpubs_are_supplied(self):
+ """
+ Same setup as the previous test, but this time the psbt's global xpubs also hold
+ the outsider's account xpub. So the parser traces every key in the output back to
+ an xpub, but the output's cosigners resolve to a list that differs from the
+ inputs' cosigners. The output is counted as a spend.
+
+ tldr: different output quorum + external xpub IN global xpubs + annotated external
+ output
+
+ No known coordinator adds an external wallet's xpub to the global xpubs, so this
+ is not expected to be seen in the real world unless someone manually edits a psbt
+ to include it.
+ """
+ for input_base64, change_hex in [
+ (PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE),
+ (PSBTTestData.MULTISIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_NESTED_SEGWIT_CHANGE),
+ (PSBTTestData.MULTISIG_LEGACY_P2SH_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE),
+ ]:
+ psbt = self._psbt_with_change(input_base64, change_hex)
+ self._repoint_at_a_different_quorum(psbt)
+
+ # Add the outsider's account xpub to the psbt's global xpubs, at the same
+ # account-level derivation path the helper used. The global xpubs now hold
+ # four: the inputs' three cosigners plus the outsider.
+ account_derivation_path = list(psbt.xpubs.values())[0].derivation
+ outsider_root = root_for_seed(PSBTTestData.recipient_seed)
+ outsider_account = outsider_root.derive(account_derivation_path)
+ psbt.xpubs[outsider_account.to_public()] = DerivationPath(outsider_root.my_fingerprint, account_derivation_path)
+ assert len(psbt.xpubs) == 4
+
+ psbt_parser = self._parse(psbt)
+
+ # Sanity check the setup: the output's cosigners resolve and differ from the
+ # inputs' cosigners. The parser keeps only the inputs' policy, so the output's
+ # is rebuilt here the same way the parser does it.
+ out = psbt.outputs[0]
+ out_policy = PSBTParser._get_policy(out, out.script_pubkey, psbt.xpubs, None)
+ input_cosigners = psbt_parser.policy["cosigners"]
+ output_cosigners = out_policy["cosigners"]
+ assert len(input_cosigners) == 3
+ assert len(output_cosigners) == 3
+ assert input_cosigners != output_cosigners
+
+ # The output should be counted as a spend.
+ assert psbt_parser.change_data == []
+ assert psbt_parser.change_amount == 0
+ assert psbt_parser.spend_amount == 10_000
+
+
def test__parse__counts_a_different_quorum_as_change_if_no_global_xpubs(self):
"""
- Same setup as the previous test, but this time the psbt omits its global xpubs.
+ Another test variation: output is once again paying a different quorum (one new
+ external xpub replacing one of the inputs' cosigners), but this time the psbt
+ omits its global xpubs.
The global xpubs are needed in order to resolve cosigners. So without them, the
inputs' cosigner list and the output's cosigner list comparison is skipped. The
- user's seed is part of the output wallet and the output's policy "shape"
+ user's seed IS part of the output wallet and the output's policy "shape"
superficially matches the input's (2-of-3, same script type), so the output is
counted as presumed change.
- BIP-174 makes the global xpubs optional and honest coordinators do omit them. The
- previous test notes that no known coordinator annotates external spend outputs so
- this test scenario is unlikely to be seen in the real world. But this version of
- the test has one notable exception: Bitcoin Core.
+ tldr: different output quorum + NO global xpubs + annotated external output
+
+ BIP-174 makes the global xpubs optional and honest coordinators do omit them.
- Core can hold the descriptors of several spending wallets. It will annotate an
- output that belongs to ANY of its descriptors, regardless of whether it differs
- from the input wallet. But Core does not write global xpubs at all, so it wasn't a
- factor in the previous test (which required the global xpubs).
+ Bitcoin Core can hold the descriptors of several spending wallets and will
+ annotate an output that belongs to ANY of its descriptors, regardless of whether
+ it differs from the input wallet. And Core never provides the global xpubs.
- But a Core-built psbt can exactly match this test's shape: a fully annotated
+ So a Core-built psbt can exactly match this test's shape: a fully annotated
foreign output and no global xpubs to compare against.
"""
# Test each multisig script typeWhy this scored 63/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.