From 45a5eabb9d3bb8afc496d219168642845c4cc8a1 Mon Sep 17 00:00:00 2001 From: kdmukai <934746+kdmukai@users.noreply.github.com> Date: Wed, 16 Sep 2026 09:30:56 -0500 Subject: [PATCH] Count an unresolved output cosigner set as a mismatch The cosigner comparison skipped any output whose cosigners failed to resolve, so the different-quorum case it exists for never fired on a real psbt: coordinators write only the spending wallet's xpubs, and a foreign cosigner's key then resolves against nothing. Once the inputs' cosigners resolve, an output that differs or fails to resolve is counted as a spend. Keys now match xpubs by derivation alone rather than requiring the claimed fingerprint to match first. The fingerprint is a label the coordinator wrote; requiring it let one mislabeled cosigner turn genuine change into a spend, and a key an xpub really derives is proof enough on its own. The different-quorum test now uses the wallet's own xpubs, the shape every xpub-writing coordinator produces, and a second test keeps the four-xpub variant. --- src/seedsigner/models/psbt_parser.py | 43 ++++++----- tests/test_psbt_parser.py | 103 +++++++++++++++++++++------ 2 files changed, 101 insertions(+), 45 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index cc2e21b3..441c1e1d 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -560,16 +560,14 @@ class PSBTParser(): 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 @@ class PSBTParser(): 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 @@ class PSBTParser(): 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) + # 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 strings so they can be sorted and compared - cosigners.append(xpub.to_base58()) - break + # 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. diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index cd179832..4d28c2ff 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -1919,6 +1919,10 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): 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 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): 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 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): 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 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): 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 - 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). + BIP-174 makes the global xpubs optional and honest coordinators do omit them. - But a Core-built psbt can exactly match this test's shape: a fully annotated + 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. + + 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 type