Page MenuHomePhabricator

[Cashtab] [OP_RETURN msg upgrade] - Pt1 - Use bytecount for unencrypted messages
ClosedPublic

Authored by emack on Apr 16 2023, 02:29.

Details

Summary

T3100

This diff updates the Send.js screen to enforce the unencrypted OP_RETURN message limit via byte size rather than char size. This utilizes the maximum OP_RETURN limit (OP_RETURN uses bytesize) and increases unencrypted message length to 215 bytes.

Fee calculation logic is also updated in this diff to take into account OP_RETURN bytesize. This could not be split into a separate diff otherwise the new bytesize parsed OP_RETURN txs will be rejected by bitcoind on account of incorrect tx fee.

The fee calc logic for token creation will be addressed in the next diff to avoid bloating this diff. It is expected to not work in this particular diff given changes to calcFee.

Updated Unit tests for getMessageByteSize, calcFee and generateTxInput.

Note: Subsequent diffs planned in this stack:

  • [Cashtab] [OP_RETURN msg upgrade] - Pt2 - Use bytecount for unencrypted airdrop messages
  • [Cashtab] [OP_RETURN msg upgrade] - Pt3 - Use bytecount for encrypted messages
Test Plan
  • npm start and navigate to the Send screen
  • Fill in valid address, amount and attempt to send an unencrypted message more than 215 bytes and ensure the Message byte size can not exceed 215 bytes error is displayed, with the Send button disabled
  • Delete the msg from above, leaving the address and amount intact, and ensure the Send button is enabled again. Send it, and note the reduced tx fee via explorer.
  • Fill in valid address, amount and attempt to send an unencrypted message less than or equal to 215 bytes. Ensure no byte size validation errors and successful broadcast of Send XEC tx. Sanity check fee on explorer, ensure the tx fee reflects the bytesize (i.e. txs with larger messages will have larger tx fees compared to txs with smaller messages).
  • Test unencrypted message with a mixture of emoji+chars, all emojis, special chars and ensure successful broadcast and tx on explorer checks out.
  • Test a one to many XEC tx with no message and ensure successful broadcast
  • Test unencrypted message on a one to many XEC tx and ensure successful broadcast

Diff Detail

Repository
rABC Bitcoin ABC
Branch
opreturnUpgrade
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23435
Build 46489: Build Diffcashtab-tests
Build 46488: arc lint + arc unit

Event Timeline

emack requested review of this revision.Apr 16 2023, 02:29
bytesofman added inline comments.
cashtab/src/components/Send/Send.js
611 ↗(On Diff #39774)

for unencrypted, call without the second parameter

1029 ↗(On Diff #39774)

Seems like this value is related to currency.opReturn.unencryptedMsgByteLimit

We should just store unencryptedMsgByteLimit and use some kind of a calculation for the airdrop value. Otherwise need to manually do the calc any time there is a change.

cashtab/src/utils/cashMethods.js
30 ↗(On Diff #39774)

In this case, don't we still need to account for other fields and not just [1]?

This revision now requires changes to proceed.Apr 19 2023, 21:05
emack marked 2 inline comments as done.Apr 20 2023, 01:14
emack added inline comments.
cashtab/src/components/Send/Send.js
1029 ↗(On Diff #39774)

Since there is a separate diff in this stack focusing on airdrop logic [Cashtab] [OP_RETURN msg upgrade] - Pt3 - Use bytecount for unencrypted airdrop messages can I propose we make this change there?

cashtab/src/utils/cashMethods.js
30 ↗(On Diff #39774)

only [1] is the message content, the other fields are just prefixes which we don't need to calculate the byte size of the message content.

emack marked an inline comment as done.

Removed optional 2nd param for unencrypted messages

bytesofman added inline comments.
cashtab/src/components/Send/Send.js
1029 ↗(On Diff #39774)

yup np

cashtab/src/utils/__tests__/cashMethods.test.js
1702 ↗(On Diff #39829)

don't specify false if there is no op_return msg, the param should be optional

cashtab/src/utils/cashMethods.js
284 ↗(On Diff #39829)

opReturnByteLength is a confusing name. We want to make sure that byteCount and length are always treated distinctly, since "front-end" length i.e. characters in a text input box does not necessarily equal "backend" byteCount.

use opReturnByteCount

379 ↗(On Diff #39829)

In this case, we don't want length, we want byteCount. Use same param name as above, opReturnByteCount

470 ↗(On Diff #39829)

The calcFee function should be designed such that, if opReturnByteCount is zero, it need not be specified.

I think it's already designed this way. Please remove this 0 specification here.

Confirm the function works without the 0 specified in its unit tests.

cashtab/src/utils/transactions.js
543 ↗(On Diff #39829)

This is a bool here but a number in generateTxInput function ... which is it?

Looks like we do not need the flag at all but do need to pass the length?

This revision now requires changes to proceed.Apr 24 2023, 22:00
emack marked 6 inline comments as done.
  • calcFee didn't work without the 0 specified for opReturnByteCount, particularly for token sends, hence adding a default false param value whereby if it was not supplied it will be set to 0 within calcFee
  • changed boolean input into default 0 bytecount for generateTxInput in transactions.js
  • various variable name changes
bytesofman added inline comments.
cashtab/src/utils/cashMethods.js
289

it's a good idea to switch cashtab fees to 1.01 sat per byte, but this should be handled by Ticker.js and is not related to this diff.

cashtab/src/utils/transactions.js
540

remove trailing whitespace

This revision now requires changes to proceed.Apr 25 2023, 13:44
emack marked 2 inline comments as done.

removed tx fee reduction logic for OP_RETURN messages and trailing whitespaces

bytesofman added inline comments.
cashtab/src/utils/cashMethods.js
284 ↗(On Diff #39950)

why not opReturnByteCount = 0

287–289 ↗(On Diff #39950)

then delete this block

This revision now requires changes to proceed.Apr 26 2023, 03:49

updated default opReturnByteCount param value to 0

bytesofman added inline comments.
cashtab/src/components/Common/Ticker.js
109 ↗(On Diff #39951)

doesn't need to be in this diff, but we should add a comment here explaining why this is the max, i.e. 220 bytes is max payload per spec, +1 for OP_RETURN, +2 for the pushdata opcodes, so 223 bytes.

Hence, a user may enter 215 bytes for a Cashtab msg because tx building will be adding 8 more bytes

6a
04
[prefix byte]
[prefix byte]
[prefix byte]
[prefix byte]
4c [next byte is pushdata byte]
[pushdata byte] (d7 for 215 on a max-size Cashtab msg)

This revision is now accepted and ready to land.Apr 26 2023, 13:32
emack marked an inline comment as done.

rebase