[#302] feat: new op return default format #332

chedieck · 2024-01-15T16:13:07Z

Related to #302

Description

Changes the prefix when adding op-return.

Instead of prefixing it with paybutton$ we prefix it to conform with the specs defined at https://reviews.bitcoinabc.org/D15176

Test plan

Make sure that when adding an opReturn to a button/widget, it gets prefixed just like I explained above.

Klakurka · 2024-01-15T18:33:19Z

Have the onSuccess and onTransaction logic check for the OP_RETURN message and only fire if it is exactly as what was specified in the URI. This will allow us to deprecate random-satoshis, or at least set it to be off by default.
I think doing just numbers and letters (eg. base62) for the unique paybutton payment code might be a bit less scary for people to see. What do you think? Also maybe better to do something that's not case sensitive since it's a URI?

chedieck · 2024-01-15T20:32:47Z

Have the onSuccess and onTransaction logic check for the OP_RETURN message and only fire if it is exactly as what was specified in the URI. This will allow us to deprecate random-satoshis, or at least set it to be off by default.

Alright, will do it in a next PR.

I think doing just numbers and letters (eg. base62) for the unique paybutton payment code might be a bit less scary for people to see. What do you think? Also maybe better to do something that's not case sensitive since it's a URI?

Yeah, I could just s.replace('+', '0').replace('/', '0') so that there are no '+' and '/' on the result; we lose some centibits of entropy but no real loss in security.

About the case sensitivess: we could just use hex (which would take 16chars to denominate 8 bytes; I used 9bytes because base64 works better with multiples of 3 bytes); which would also solve the other problem about the symbols. Nonetheless, case-sensitiviness shouldn't be an issue for URIs as specified by RFC-3986; the only parts of the URI that are assumed to be case-insensitive are the scheme and the host.

But I do think that hex, as the industrial standard for most things, is more readable and less scary for users so we can just go with that; the only loss is the more usage of chars when saving it to the database; but that's is also negligible

Fabcien · 2024-01-17T22:07:36Z

react/src/util/opReturn.ts

+  const wordArray = lib.WordArray.random(8);
+
+  // Convert the word array to a hex string
+  const hexString = enc.Hex.stringify(wordArray);


Why do you need stringToHex if you have Hex.stringify doing the same thing ?

stringToHex takes a string, while Hex.stringify takes a WordArray, which is used only here in the context of a cryptographically safe random chunk of bytes

Fabcien · 2024-01-17T22:08:43Z

react/src/util/opReturn.ts

+  // a hex character encodes 4 bits of information (2⁴ = 16);
+  // ... therefore; 8 bytes = 72 bits => 72/4 = 16 hex chars
+  // + 1 byte of push data at the beggining = 2 hex chars
+  // = 18 chars


This is super confusing. 72/4 != 16, and 8 bytes is not 72 bits

Indeed, this was a different math at some point (I was using base64 and, therefore, 9 bytes, to avoid padding) and I forgot to change all the numbers. Fixed.

About as it being confusing, I think is just because is overly technical? I just left as an explanation as of why is it 18 chars long, which I decided to put on a comment since I've confused number of bytes with number of chars a couple of times when working on this issue.

Fabcien · 2024-01-17T22:13:25Z

react/src/util/opReturn.ts

+    );
  }
-  return OP_RETURN_PREFIX + opReturn;
+  let pushData = bytesQuantity.toString(16).padStart(2, '0')


This doesn't work for push datas > 75 bytes, so the above limit check makes no sense

Why not?

This doesn't work when bytesQuantity > 255, but the limit above makes it not a problem

Why not?

There are some obscure rules to how script works in bitcoin. To the point where it's probably worth using a script library, like the one in bitcoinjs-lib or BitGo's utxo-lib, to build any script strings.

https://en.bitcoin.it/wiki/Script

You can't push more than 0x4b bytes with one byte of pushdata. 0x4c is a special byte in script that means "the next byte is one byte of pushdata." 0x4d is a special byte in script that means "the next two bytes are two bytes of pushdata."

imo your options here are

Do not use a library, but restrict pushes to between 1 and 75 bytes (seems like you are only coding for 2 pushes here that are always 8 or 9 bytes, so this is probably ok for now though not robust).

Use bitcoinjs-lib (or copy what Cashtab has done with utxo-lib), and have the pushdata created automatically with script.push

Thanks for the info, this also answers the other question I had about why the need for two bytes of pushdata if it's always < 255.

But then I see two cases:

data < 76 bytes; single byte for push data;

76 <= data < 205 bytes; two bytes for push data: 4c + the actualy byte quantity

Since we don't have to care about bigger amounts than 255 (that would require more than two bytes of pushdata info, namely using the 4d and 4e op codes), this is still fairly simple to implement; I've made the changes to take that into account

Klakurka · 2024-01-17T22:17:57Z

@chedieck can u update the PR desc w/ the actual spec as it's currently just showing what we were planning originally.

BytesOfMan · 2024-01-17T22:48:18Z

react/src/util/opReturn.ts

+  // ... therefore; 8 bytes = 72 bits => 72/4 = 16 hex chars
+  // + 1 byte of push data at the beggining = 2 hex chars
+  // = 18 chars
+  return `08${hexString}`;


It is perhaps nice to avoid using a library like utxolib or the Buffer object, which Cashtab uses to create OP_RETURN outputs.

However, if you are going to avoid those libs, would still recommend a more flexible way of creating OP_RETURN outputs.

A function that only makes a substring for a certain type of spec is not very useful if the spec ever changes. Ideally you have a function like the script.push here, where the correct pushdata is added automatically based on the data you are pushing.

Yeah, Buffer doesn't work well when bundling.

I created a function to prepend a pushData to a hex string, more clean indeed.

BytesOfMan · 2024-01-17T22:51:04Z

react/src/util/opReturn.ts

+// - 1 for the version byte: '\x00'
+// - 9 from the 8-byte nonce with push data
+// = 205 available bytes
+export const BYTES_LIMIT = 220 - 5 - 1 - 9; // 205


BYTES_LIMIT could be more specifically named. Here, we are talking about the available bytes for the data pushed described in this draft spec.

ok I think I am misreading what BYTES_LIMIT is for here. looks like remaining bytes that could be available if the tx is to spec? I don't get it.

It's the limit for the data the user himself wants to put on the paybutton. I renamed it to make it more clear.

BytesOfMan · 2024-01-17T22:52:57Z

react/src/util/opReturn.ts

+export const OP_RETURN_PREFIX = '50415900'; // PAY\x00
+export const VERSION = '00'; // \x00
+
+// 220 total bytes


223 total bytes

1 for OP_RETURN 6a
1 for 04 lokad id pushdata
4 for lokad id
1 for OP_0 version byte
...
per draft spec, the 8-byte nonce is optional and not always present? So sometimes these bytes are available for a data push?

1 or 2 pushdata bytes for the data push depending on size of data

...remainder for data

Fixed the math; thanks

Also did the division to better clarify what is being subtracted.

As for the 'optionality' of the 8-byte nonce, indeed I'll implement this in another PR (it will require a new set of props, checks, etc so I prefer to do it separately)

pushData will never be more than one byte because the data will never be bigger than 255 bytes; since the script it self is limited to less than that.

BytesOfMan · 2024-01-17T22:55:05Z

react/src/util/opReturn.ts

+//`04` - Pushdata of the Protocol Identifier, `50415900`
+//`50415900` - PayButton protocol identifier, ascii "PAY"
+//`00` - Version 0, pushed as `OP_0`
+//`09` - pushdata for the data payload, signifying this tx has 9 bytes of data


is this always 9 bytes? if so I should update the spec. We can always go to another version if you have more than 9 bytes of data, but I can imagine wanting the flexibility of data as arbitrary available bytes without needing to go to another version.

Well it should be variable given that that's the main reason for bothering to make the nonce / payment ID optional (to claw back some bytes if they really really want it).

Sorry, I copied your example (made some changes) but forgot to paste the example itself and... the word 'example'.

I thought the "signifying" implied the variability, but it's very subtle indeed.

Redid the text explanation; moved it to the top of the main function at the bottom.

react/src/util/opReturn.ts

BytesOfMan · 2024-01-17T22:57:11Z

Difficult to do what these functions do and how they succeed or fail without any unit tests

chedieck · 2024-01-17T23:41:14Z

@chedieck can u update the PR desc w/ the actual spec as it's currently just showing what we were planning originally.

Done

chedieck · 2024-01-18T00:31:56Z

Difficult to do what these functions do and how they succeed or fail without any unit tests

Yeah, I agree. However, this repo (paybutton client) doesn't have any unittests whatsoever, so this would be a task in itself.

The paybutton-server repo does, and I have already implemented the unittests for the server there (will open a PR on that soon)

In any case, this is the main reason I have been overly verbose with the comments here.

Klakurka · 2024-01-18T01:12:41Z

Yeah client testing is planned for this phase: #32

BytesOfMan · 2024-01-19T01:16:18Z

react/src/util/opReturn.ts

+// - 8 from the 8-byte nonce
+// - 2 from the maximum size for the data pushdata
+// = 205 available bytes
+export const USER_DATA_BYTES_LIMIT = 223 - 1 - 1 - 4 - 1 - 1 - 8 - 2; // 205


don't need to calculate it every time, 205 is good since documented

BytesOfMan · 2024-01-19T09:08:13Z

react/src/util/opReturn.ts

+// function below constructs it in the following way:
+//
+// `04` - Pushdata of the Protocol Identifier, `50415900`
+// `50415900` - PayButton protocol identifier, ascii "PAY"


was this finalized? it's in the draft spec but was PAYB the winner?

I think we closed on PAY\x00 but @Klakurka can confirm

Yep, just going with the suggested one is fine.

Klakurka · 2024-01-19T19:03:54Z

@chedieck Possible to swap the 'Depends on' so that this depends on #340 being merged first, then this branch be rebased w/ it?

chedieck · 2024-01-19T19:50:31Z

@chedieck Possible to swap the 'Depends on' so that this depends on #340 being merged first, then this branch be rebased w/ it?

That branch is this branch + tests and some changes (related to problems found when creating the tests), so if that branch is merged this is automatically merged too (it will have an empty diff)

Klakurka · 2024-01-19T20:25:07Z

@chedieck Merge conflicts.

chedieck · 2024-01-19T20:46:19Z

@chedieck Merge conflicts.

Solved.

chedieck · 2024-01-21T05:51:29Z

Closing this since all its changes have already been merged in another PR

Klakurka self-requested a review January 15, 2024 18:21

Klakurka added the enhancement (UI/UX/feature) New feature or request label Jan 15, 2024

Fabcien reviewed Jan 17, 2024

View reviewed changes

BytesOfMan reviewed Jan 17, 2024

View reviewed changes

Klakurka reviewed Jan 17, 2024

View reviewed changes

react/src/util/opReturn.ts Outdated Show resolved Hide resolved

Klakurka requested a review from Fabcien January 17, 2024 23:42

chedieck requested review from BytesOfMan and Klakurka January 18, 2024 00:32

chedieck mentioned this pull request Jan 18, 2024

Have the onSuccess and onTransaction check for OP_RETURN and only fire on exact match #338

Closed

chedieck mentioned this pull request Jan 18, 2024

[#774] feat: op return new specs PayButton/paybutton-server#789

Merged

1 task

chedieck force-pushed the feat/new-op-return-default-format branch from 511a87a to aa2dc09 Compare January 18, 2024 20:07

BytesOfMan reviewed Jan 19, 2024

View reviewed changes

chedieck mentioned this pull request Jan 19, 2024

[#302] test: op return specs tests #340

Merged

2 tasks

Merge branch 'master' into feat/new-op-return-default-format

c9a1f0f

Merge branch 'master' into feat/new-op-return-default-format

47482fe

chedieck closed this Jan 21, 2024

[#302] feat: new op return default format #332

[#302] feat: new op return default format #332

Uh oh!

Conversation

chedieck commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test plan

Uh oh!

Klakurka commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chedieck commented Jan 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klakurka commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BytesOfMan commented Jan 17, 2024

Uh oh!

chedieck commented Jan 17, 2024

Uh oh!

chedieck commented Jan 18, 2024

Uh oh!

Klakurka commented Jan 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klakurka commented Jan 19, 2024

Uh oh!

chedieck commented Jan 19, 2024

Uh oh!

Klakurka commented Jan 19, 2024

Uh oh!

chedieck commented Jan 19, 2024

Uh oh!

chedieck commented Jan 21, 2024

chedieck commented Jan 15, 2024 •

edited

Loading

Klakurka commented Jan 15, 2024 •

edited

Loading