Page MenuHomePhabricator

[ecash-herald] Cleaner OP_RETURN parsing
ClosedPublic

Authored by bytesofman on Aug 16 2023, 23:00.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC0fee7f45e083: [ecash-herald] Cleaner OP_RETURN parsing
Summary

Some OP_RETURN txs of unknown schema are printed to the channel in an unintelligible way. Many times, assuming an unknown OP_RETURN is ascii can make it readable (existing test vector for unknown tx is like this).

Other times, decoding a random non-ascii hex string as ascii creates jargon.

Add a function containsOnlyPrintableAscii to determine if a given OP_RETURN hex string contains only non-control ascii characters. If a given OP_RETURN stack array does not, print each push as a hex value.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested changes to this revision.Aug 17 2023, 08:03
Fabien added a subscriber: Fabien.

Would that prevent memo strings with emojis?

apps/ecash-herald/src/utils.js
362 ↗(On Diff #41837)

This is not testing if the string is *probably* not ASCII, any char outside the ASCII range will cause the function to return false.

This revision now requires changes to proceed.Aug 17 2023, 08:03
bytesofman marked an inline comment as done.

Rename isProbablyAscii to containsOnlyAscii

Would that prevent memo strings with emojis?

It's only being applied on "unknown" OP_RETURN txs -- i.e. OP_RETURN txs that do not include a protocol identifier understood by ecash herald. memo txs have a parsed protocol identifier and are handled separately in parseOpReturn and parseMemoOutputScript. Memo txs that Cashtab has not been programmed to handle print Unknown memo action.

So -- if a memo tx is not properly prefixed as a memo tx per memo spec, it could be caught as an unknown tx. In this case, it would be rendered either as ascii parsed if it happened to only contain ascii characters (wrong for memo, which is utf8, though would probably appear mostly intelligible) -- or as a string of hex values prefixed by 0x and separated by a space if any values were non-ascii.

imo an improvement over the current app which will just treat it as ascii no matter what.

Fabien requested changes to this revision.Aug 18 2023, 10:00

On second thought ascii is probably not what you want, you want it to be printable and human readable. so 32 <= byte <= 126 (plus maybe CR and LF)

This revision now requires changes to proceed.Aug 18 2023, 10:00

Also reject ascii control characters

update function def comments

Please update the summary before you land

This revision is now accepted and ready to land.Aug 19 2023, 06:26
This revision was automatically updated to reflect the committed changes.