Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement] Parse psbts with OP_RETURN data & display payload #517

Merged
merged 15 commits into from
Jul 10, 2024

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Jan 6, 2024

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 OP_RETURN 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:

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

This pull request is categorized as a:

  • New feature

Checklist

  • 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?

  • Yes

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

@@ -182,6 +182,10 @@ def _parse_outputs(self):
})
self.change_amount += self.psbt.tx.vout[i].value

elif self.psbt.tx.vout[i].value == 0 and self.psbt.tx.vout[i].script_pubkey.script_type() is None and self.psbt.tx.vout[i].script_pubkey.data:
# OP_RETURN
self.op_return = self.psbt.tx.vout[i].script_pubkey.data[3:].decode('UTF-8')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering what these first 3 bytes are?

  • op_return?
  • compact size?
  • ???
    Can we assert some of them... or use them just above to know for sure it is op_return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I was being so dumb!!

First 3 bytes = OP_RETURN + OP_PUSHDATA1 + len(payload)

Updated in latest commit.

@kdmukai kdmukai marked this pull request as ready for review January 8, 2024 21:34
@kdmukai kdmukai changed the title [DRAFT] [New Feature] Parse psbts with OP_RETURN data & display payload [Enhancement] Parse psbts with OP_RETURN data & display payload Jan 8, 2024
@kdmukai
Copy link
Contributor Author

kdmukai commented Mar 4, 2024

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
Copy link

jdlcdl commented Jul 5, 2024

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).

@kdmukai kdmukai force-pushed the parse_op_return branch from fc97d5d to 03ebf7f Compare July 9, 2024 12:50
@jdlcdl
Copy link

jdlcdl commented Jul 9, 2024

Re ran same tests as above.

as of df7bc42
ACK tested

@newtonick
Copy link
Collaborator

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
Copy link
Collaborator

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.

@newtonick newtonick merged commit 724033c into SeedSigner:dev Jul 10, 2024
2 checks passed
@petertodd
Copy link

petertodd commented Jul 10, 2024 via email

@kdmukai kdmukai deleted the parse_op_return branch September 2, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged Not Yet Released
Development

Successfully merging this pull request may close these issues.

4 participants