[Enhancement] Parse psbts with OP_RETURN data & display payload

kdmukai commented 8 months ago

Description

v0.7.0 currently cannot parse a psbt that includes an OP_RETURN due to a few assumptions in the PSBTParser.

This PR:

Adds explicit support for parsing OP_RETURN outputs.
- _The approach used here to detect an OPRETURN output definitely needs more eyeballs and scrutiny.
Updates the PBST Overview pictogram to include the OP_RETURN output.
Adds an OP_RETURN screen to the PSBT review flow to display the payload
- Human-readable text (all bytes can be decoded as UTF-8) will be displayed as-is.
- If any bytes can't be decoded to UTF-8, a "raw hex data" label is added and the data is displayed in full in hex (per @petertodd)
Adds a PSBTParser test for an OP_RETURN output
Adds flow-based test to route through the new PSBTOpReturnView
Adds the following three new screenshots:

How to test

There aren't any tools I'm aware of that make it easy to construct a psbt with an OP_RETURN. So we have to build the psbt ourselves:

Create an initial psbt in Sparrow
Export it via File -> Save PSBT -> To Clipboard -> As Base64
Paste the psbt into the embit snippet here
Paste the resulting psbt back into Sparrow: File -> Open Transaction -> From Text
From here you can use Sparrow as usual to display the psbt as a QR code

Remaining steps:

[x] Test how the display handles a full 80-byte OP_RETURN
[x] Test how the display handles non-human readable data
[x] Add PSBTParser test w/an OP_RETURN output
[x] Add screenshots in the PSBT review flow with OP_RETURN data

This pull request is categorized as a:

[x] New feature

Checklist

[x] I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

[x] Yes

I have tested this PR on the following platforms/os:

[x] Raspberry Pi OS Manual Build

kdmukai commented 6 months ago

Note that non-human readable OP_RETURN data has now been changed to be displayed just as raw hex instead of attempting to interpret it into nonsense UTF-8.

See updated screenshot in PR description.

jdlcdl commented 2 months ago

ACK Tested as of fc97d5d: (I now see that merge of 495 may have created conflicts... will re-check this as soon as conflicts are resolved).

regarding >>"The approach used here to detect an OP_RETURN output definitely needs more eyeballs and scrutiny.", I believe that the first byte as op_return is the most obvious way to identify these.

All tests done on dev-rpi0, with multiple single-sig native-segwit inputs paying multiple outputs (legacy+nestedsegwit+nativesegwit+taproot+multisig-native-segwit) + op_return payloads of:

test:seedsigner/pull/517 (less than 80 text chars)
test:seedsigner/pull/517 6789012345678901234567890123456789012345678901234567890 (80 chars w/o convenient wrap)
test:seedsigner/pull/517 678901234567890123456789 1234567890123456789 1234567890 (80 chars w/ convenient wrap)
test:seedsigner/pull/517 678901234567890123456789 1234567890123456789 1234567890 This text exceeds 80 characters
5631 raw bytes of "jdlcdl.png" (my round dark-to-light-blue JD avatar).

SeedSigner always identified self transfer and change correctly, displayed the payload (as text when possible) with wrapping when convenient else with overflow off screen (raw bytes were wrapped to a full screen of hex), and the signed tx was always readable and publishable when it got back to Sparrow (but my testnet3 node denied the last one based on not enough fees).

jdlcdl commented 2 months ago

Re ran same tests as above.

as of df7bc42 ACK tested

newtonick commented 2 months ago

ACK and tested. I think there are some corner cases not covered in this PR, however since this is such a niche feature for advanced users I'm good with addressing those as the need arises.

newtonick commented 2 months ago

example of a corner case: https://mutinynet.com/tx/dbb10d3224f36c85b1d45751c88348a40774f93f3d2c43214db72a09c8c17c27

With the functionality in this PR, only the last OP_RETURN message is displayed even though the transactions has 2 OP_RETURN messages. However most nodes would reject this transaction because of network rules.

I don't think this needs to be fixed. Just pointing it out.

petertodd commented 2 months ago

On Tue, Jul 09, 2024 at 06:00:15PM -0700, Nick Klockenga wrote:

example of a corner case: https://mutinynet.com/tx/dbb10d3224f36c85b1d45751c88348a40774f93f3d2c43214db72a09c8c17c27

With the functionality in this PR, only the last OP_RETURN message is displayed even though the transactions has 2 OP_RETURN messages. However most nodes would reject this transaction because of network rules.

FWIW Libre Relay nodes will relay such transactions, and F2Pool (and probably others) mine them.

I don't think this needs to be fixed. Just pointing it out.

Since this is a signing application, I agree that it's fine to just say multiple OP_Returns aren't supported yet.

SeedSigner / seedsigner