Skip to content

Parse the OP_RETURN push opcode instead of assuming OP_PUSHDATA1 - #1042

Open
Chaitanya-Keyal wants to merge 3 commits into
SeedSigner:devfrom
Chaitanya-Keyal:op-return-push-opcode
Open

Chaitanya-Keyal wants to merge 3 commits into
SeedSigner:devfrom
Chaitanya-Keyal:op-return-push-opcode

Conversation

@Chaitanya-Keyal

@Chaitanya-Keyal Chaitanya-Keyal commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Closes #963 and supersedes #965 and #1003. Both fix the same bug and can be closed once this lands. @ruipereira1 and @kwsantiago have been added as co-authors on the relevant commits.

Description

Problem or Issue being addressed

PSBTParser._parse_outputs() read the OP_RETURN payload at a fixed offset:

if self.psbt.tx.vout[i].script_pubkey.data[0] == OPCODES.OP_RETURN:
    # The data is written as: OP_RETURN + OP_PUSHDATA1 + len(payload) + payload
    self.op_return_data = self.psbt.tx.vout[i].script_pubkey.data[3:]

That comment is only true for payloads of 76 bytes and up. Script push opcodes are variable width:

encoding prefix used for
direct push (0x01-0x4b) 2 bytes payloads up to 75 bytes
OP_PUSHDATA1 3 bytes 76 to 255 bytes
OP_PUSHDATA2 4 bytes 256 to 65535 bytes
OP_PUSHDATA4 6 bytes larger

Bitcoin Core emits the shortest encoding that fits, so most real OP_RETURNs are a direct push and [3:] eats their first byte:

scriptPubKey:  6a 28 "Chancellor on the brink of third bailout"
parsed:           b'hancellor on the brink of third bailout'
expected:         b'Chancellor on the brink of third bailout'

Why the test suite did not catch it

Every existing fixture hard-coded OP_PUSHDATA1, the one encoding the parser assumed:

  • tests/screenshot_generator/generator.py:85-89 concatenated it unconditionally. Its text payload is 50 bytes, which Core would push directly, so the screenshot was right for the wrong reason. Its other payload is 80 bytes, where OP_PUSHDATA1 really is minimal. No fixture ever produced a direct push.
  • The regtest PSBT behind test_parse_op_return_content was built the same way: 2b 6a 4c 28 43 68 61 6e ....

Solution

Read the push opcode and slice to the length it declares, in a static PSBTParser._parse_op_return_payload() helper instead of inline in the loop. All four encodings are handled.

Two judgement calls, both explained in the docstring:

  • Multiple pushes are concatenated. Legal but unusual. The user is approving the data the transaction commits to, and push boundaries have no consensus meaning or display convention.
  • Malformed scripts show their bytes rather than nothing. If a byte is not a data push, or a push overruns the script, the walk stops and the rest is appended as-is. Returning b"" would hide committed data from a review screen.

Also fixed on the same lines:

  • op_return_data is reset in _parse_outputs. It was only set in __init__, so re-parsing an instance kept a stale payload.
  • The leading byte is compared by slice, so an empty scriptPubKey returns b"" instead of raising IndexError.

Additional Information

Left for a follow-up PR (#1043), to keep this one to a single change. None of these depend on push encoding; they are separate defects in the same area:

  • Multiple OP_RETURN outputs collapse to the last one, since op_return_data is one bytes the loop overwrites. Core v30 dropped the one-per-transaction relay limit.
  • An OP_RETURN's value is added to no total, so the overview's inputs = spend + change + fee does not balance when sats are burned on one.
  • PSBTOpReturnScreen draws the whole payload into one unbounded TextArea. Not just cosmetic: it reflows the entire payload before drawing anything, at quadratic cost. 1 KB takes 83ms, 4 KB takes 1.2s, 16 KB takes 15.8s on a dev machine, and the device is much slower. At Core's new default carrier size the screen is effectively unreachable.

Screenshots

No new or modified screens, however the existing screenshot receives the correct bytes now.


This pull request is categorized as a:

  • Bug fix

Checklist

I ran pytest locally

  • All tests passed before submitting the PR

I included screenshots of any new or modified screens

  • N/A

I added or updated tests

  • Yes

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

Tested with my fork of Keith's testqrs.com (kdmukAI-bot/btc-datagen#2)


I have reviewed these notes:

  • Keep your changes limited in scope.
  • If you uncover other issues or improvements along the way, ideally submit those as a separate PR.
  • The more complicated the PR, the harder it is to review, test, and merge.
  • We appreciate your efforts, but we're a small team of volunteers so PR review can be a very slow process.
  • Please only "@" mention a contributor if their input is truly needed to enable further progress.
  • I understand

Chaitanya-Keyal and others added 3 commits September 23, 2026 11:45
The payload was read at a fixed 3-byte offset, which only holds when the script
uses OP_PUSHDATA1. Opcodes 0x01-0x4b are themselves the byte count, and that is
the minimal encoding Bitcoin Core emits, so every payload of 75 bytes or fewer
lost its first byte:

    scriptPubKey  6a 28 "Chancellor on the brink of third bailout"
    parsed        b'hancellor on the brink of third bailout'

PSBTOpReturnView is a pre-signing review screen, so the user approved a
transaction whose OP_RETURN content was displayed wrong. When the payload is not
human-readable it renders as hex, where a one byte shift is invisible.

Read the push opcode and slice to the declared length, in a static helper so the
loop stays readable. OP_PUSHDATA2 and OP_PUSHDATA4 are handled too: Bitcoin Core
v30 raised the default -datacarriersize to 100,000 bytes, and consensus never
limited OP_RETURN data at all, so a payload past what OP_PUSHDATA1 can describe
is no longer exotic.

Several pushes after OP_RETURN are legal and are concatenated; the user is being
shown the data the transaction commits to, and the push boundaries carry no
consensus meaning. Anything that is not a data push, or a push that overruns the
script, ends the walk and the remaining bytes are surfaced verbatim rather than
dropped.

Also reset op_return_data in _parse_outputs, which only ever set it in
__init__, and compare the first byte by slice so an empty scriptPubKey does not
raise IndexError.

Fixes SeedSigner#963

Co-authored-by: ruipereira1 <159703278+ruipereira1@users.noreply.github.com>
Co-authored-by: kwsantiago <44934418+kwsantiago@users.noreply.github.com>
Every existing OP_RETURN vector hand-built OP_PUSHDATA1, which is exactly the one
encoding the parser assumed, so the mis-slice never showed up. The regtest psbt
behind test_parse_op_return_content carries 2b 6a 4c 28 43 68 61 6e ... and the
screenshot generator concatenated OP_PUSHDATA1 by hand.

Add fixture helpers that build an OP_RETURN scriptPubKey with the minimal push by
default, and that can force a specific OP_PUSHDATA* so the non-minimal encodings
a coordinator is free to produce are testable too. Note in the helper that the
scriptPubKey and value have to be set on the OutputScope: PSBT.tx rebuilds the
transaction from the scopes on every access, so assigning through psbt.tx.vout[i]
is silently discarded and leaves a test asserting against the fixture it meant to
replace.

The new tests cover the direct-push/OP_PUSHDATA1 boundary at 75/76, OP_PUSHDATA2
and OP_PUSHDATA4, a non-minimal encoding, several pushes in one script, a bare
OP_RETURN, an empty scriptPubKey, a truncated length field, an over-long declared
length, and a leading opcode that is not a data push.

Co-authored-by: ruipereira1 <159703278+ruipereira1@users.noreply.github.com>
Co-authored-by: kwsantiago <44934418+kwsantiago@users.noreply.github.com>
The generator concatenated OP_PUSHDATA1 unconditionally, so its 49-byte payload
was encoded a way Bitcoin Core would never produce. Push directly at 75 bytes or
fewer and via OP_PUSHDATA1 above that, matching the parser. The rendered
screenshots are unchanged; the vector behind them is no longer wrong.

Co-authored-by: kwsantiago <44934418+kwsantiago@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 0.8.8 Needs Review

Development

Successfully merging this pull request may close these issues.

OP_RETURN payload is mis-parsed when the minimal push opcode is used

2 participants