Skip to content

Reject PSBT inputs that carry no utxo data - #966

Open
ruipereira1 wants to merge 2 commits into
SeedSigner:devfrom
ruipereira1:fix/psbt-input-missing-utxo
Open

Reject PSBT inputs that carry no utxo data#966
ruipereira1 wants to merge 2 commits into
SeedSigner:devfrom
ruipereira1:fix/psbt-input-missing-utxo

Conversation

@ruipereira1

@ruipereira1 ruipereira1 commented Aug 1, 2026

Copy link
Copy Markdown

Note: this touches the same region of psbt_parser.py as #965, so it is based on
that branch and includes its commit. Only the second commit
("Reject PSBT inputs that carry no utxo data") belongs to this PR — please review #965
first. Happy to rebase once that one lands, or to fold the two into a single
"PSBT parser hardening" PR if you'd prefer fewer moving parts.

Description

Problem or Issue being addressed

PSBTParser._parse_inputs() assigns script_pubkey from whichever utxo field an input
carries:

for inp in self.psbt.inputs:
    if inp.witness_utxo:
        self.input_amount += inp.witness_utxo.value
        script_pubkey = inp.witness_utxo.script_pubkey
    elif inp.non_witness_utxo:
        self.input_amount += inp.utxo.value
        script_pubkey = inp.script_pubkey

    inp_policy = PSBTParser._get_policy(inp, script_pubkey, self.psbt.xpubs)

An input with neither field leaves the local variable bound to the previous iteration's
value, so that input's policy is derived from a different input's script. On the first
input it raises UnboundLocalError instead — confirmed by the added test, which strips
witness_utxo/non_witness_utxo from a single-input PSBT.

Solution

An explicit else that raises, rather than reading a stale value.

This is defensive rather than a fix for an observed failure: psbt.fee() later in
_parse_outputs() would most likely raise first on such a PSBT. But silently reusing the
last loop value does not belong in the code that decides whether a transaction has mixed
inputs.

Additional Information

The error message is short because Controller.handle_exception renders it on the error
screen.

Screenshots

No new or modified screens.


This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

I ran pytest locally

  • All tests passed before submitting the PR
  • I couldn't run the tests
  • N/A

187 passed with this change. 4 tests in test_seedqr.py that decode QR images could not run in my environment because libzbar does not load on Windows; they fail identically on an unmodified dev checkout (baseline: 184 passed, same 4 failures) and are unrelated to this change. They pass in CI, which installs libzbar0.


I included screenshots of any new or modified screens

  • Yes
  • No
  • N/A

I added or updated tests

  • Yes
  • No, I'm a fool
  • N/A

I tested this PR hands-on on the following platform(s):

  • Raspberry Pi OS Manual Build
  • SeedSigner OS on a Pi0/Pi0W board
  • Emulator

Not tested on hardware or the emulator — I don't have a device. Covered by a unit test. Happy for a maintainer to verify on-device.


I have reviewed these notes:

  • I understand

The parser assumed the payload is always preceded by OP_PUSHDATA1 and sliced
the scriptPubKey at a fixed offset of 3. That encoding is only required for
payloads of 76-80 bytes; anything up to 75 bytes uses a single-byte direct
push, which is what Bitcoin Core emits. For those the fixed offset drops the
payload's first byte, so the OP_RETURN review screen shows the user data that
differs from what they are about to sign.

Read the push opcode instead of assuming its width, and slice to the declared
length so trailing bytes are not included.

The existing test vectors all encode OP_PUSHDATA1 explicitly, which is why
this was not caught.
`_parse_inputs` sets `script_pubkey` from whichever utxo field the input has.
An input with neither leaves the local variable bound to the previous
iteration's value, so `_get_policy()` evaluates that input against a different
input's scriptPubKey. On the first input it raises UnboundLocalError instead.

Raise explicitly rather than reading a stale value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants