Restore why change candidacy ignores cosigners
What the developer wrote
Authored by kdmukai
Restore why change candidacy ignores cosigners
Renaming _policy_shape_matches to _is_change_candidate dropped the
reason the candidacy test compares shape alone. Without it, adding the
cosigners to the comparison reads as a harmless tightening.
The old wording named a misannotated fingerprint as what breaks an
output's cosigner resolution. Since #1032, _get_cosigners matches each
key to a global xpub by derivation path alone and never reads the
fingerprint, so the restored note names a misannotated derivation path
instead.
What changed, and why it matters
This commit only adds explanatory comments to a function that decides whether a Bitcoin transaction output should be treated as 'change' (money going back to the user's own wallet). The code itself is not changed. The new comments warn that comparing cosigner details at this early stage could let a malicious or malformed PSBT (transaction file) hide an output from normal change verification, because one bad derivation path could prevent cosigners from matching. The commit restores documentation that was accidentally lost during an earlier rename, so future developers do not mistakenly 'tighten' this check by adding cosigners here.
No immediate action is required for this commit because it changes only comments. However, reviewers should verify that the actual _is_change_candidate implementation still correctly ignores cosigners and that downstream cosigner validation is performed as described. Consider adding a regression test or code comment guard so the rationale is not lost again in future refactors.
Security signals we found
Comment-only change restoring a security rationale for ignoring cosigners in change candidacy
Describes a potential bypass where a malformed PSBT could cause an output to skip change-level scrutiny
References prior behavior change in PR #1032 regarding fingerprint vs derivation-path cosigner resolution
Evidence from the diff
The diff is comment-only in src/seedsigner/models/psbt_parser.py inside _is_change_candidate(). A previous refactor renamed _policy_shape_matches to _is_change_candidate and dropped the rationale for comparing policy shape alone and ignoring cosigners. This commit restores that rationale and updates it: since PR #1032, _get_cosigners resolves keys to global xpubs by derivation path only, so the restored note now blames a misannotated derivation path rather than a misannotated fingerprint. No executable logic changes.
Changed components
src/seedsigner/models/psbt_parser.py_is_change_candidate() methodInspect captured patch +10 / −1
### src/seedsigner/models/psbt_parser.py
@@ -728,7 +728,16 @@ def _is_change_candidate(self, out: OutputScope, out_policy: dict) -> bool:
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.
+ cosigners into account; comparing the cosigners here would let a psbt decide which
+ of its own outputs get verified:
+ * One misannotated derivation path would make that output's cosigners fail
+ to resolve.
+ * The output's missing cosigners would mean that it would not match the inputs'
+ cosigners.
+ * End result: the output would not be considered a change candidate and would
+ not go through the same scrutiny that change candidates do.
+
+ Cosigner information, if provided, is evaluated later.
"""
# The outlier: a single sig p2sh output when the inputs are p2sh-p2wpkh.
if (Why this scored 35/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.