-
Notifications
You must be signed in to change notification settings - Fork 177
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
Conversation
src/seedsigner/models/psbt_parser.py
Outdated
@@ -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') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
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:
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). |
Re ran same tests as above. as of df7bc42 |
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. |
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. |
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.
|
Description
v0.7.0 currently cannot parse a psbt that includes an OP_RETURN due to a few assumptions in the PSBTParser.
This PR:
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:
Remaining steps:
This pull request is categorized as a:
Checklist
pytest
and made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: