Recognize inscriptions that use pushnum opcodes #2497
Conversation
casey
requested review from
veryordinally and
raphjaph
and removed request for
veryordinally
| Some(Instruction::PushBytes(push)) => { | ||
| payload.push(push.as_bytes().to_vec()); | ||
| } |
There was a problem hiding this comment.
Perhaps there should be a flag that is flipped on each instruction called is_tag and when is_tag is true and it parses a PushBytes, it should reject bytes greater than 4 bytes long and parse the bytes as a little endian signed magnitude integer. Then check that the number meets requirements of the protocol and push the standard representation (vec![n]) onto the payload.
If is_tag is false, then just push the payload as-is. (current behavior)
While supporting PUSHNUM opcodes is a great first step, Bitcoin Core can support up to a 4 byte pushdata as integers when performing math/comparison ops.
My is_tag solution is predicated on the fact that starting with the "ord" push, the pattern seems to always be alternating (starting with false on "ord")
There was a problem hiding this comment.
See also: rust-bitcoin/rust-bitcoin#2081 (I think this will be in the next version release)
There was a problem hiding this comment.
The behavior, with this PR, is that you can use pushnum opcodes, or, the minimal encoding of the integer using a pushdata opcode. This is done because inscriptions were made using pushnum opcodes, back when our docs were incorrect, and used pushnum opcodes incorrectly, even though they weren't recognized.
I think the only difference in behavior between the above behavior, and what you're suggesting, is that we would additionally recognize inscriptions that use the non-minimal encoding of integers, e.g. with trailing zeros, using pushdata opcodes. I'm not sure I see the benefit in doing that.
There was a problem hiding this comment.
Where is the official protocol documentation?
According to https://docs.ordinals.com/inscriptions.html
OP_FALSE
OP_IF
OP_PUSH "ord"
OP_PUSH 1
OP_PUSH "text/plain;charset=utf-8"
OP_PUSH 0
OP_PUSH "Hello, world!"
OP_ENDIF
This isn't very specific. There is no such thing as OP_PUSH, so people can possibly take it to mean "as long as the Bitcoin Script interpreter would see it as a 1, that's ok." This misunderstanding is the core of why people thought OP_1 (0x51) was ok, because of this ambiguity.
(In addition, the character encoding of "ord" and "text/plain;charset=utf-8" are not defined. Since we push byte arrays onto the stack and not strings, defining the character encoding is important... I left out the "Hello, world!" because one could argue that the push must follow the MIME type, which specifies utf-8 in the example, so that should be fine.)
In order to accommodate all interpretations of the spec, all representations of the number 1 should be accounted for, which is why I commented.
Alternatively, you could make the spec more explicit, and explicitly forbid push numbers beyond 1 byte in size.
There was a problem hiding this comment.
Notably, this limitation also limits the number of possible tags to 255 (excluding 0 due to its ambiguity with OP_0 aka &[])
Not super important, 255 is probably more than enough, but just thought I'd mention.
There was a problem hiding this comment.
I think it would be better to improve the spec, rather than relax the implementation.
There was a problem hiding this comment.
- Specify "ord" and any MIME types for tag=1 MUST be utf8. Maybe even mention the 3 bytes for "ord" as an example.
- Specify that the 0 tag must use OP_0 (which is the 0 pushbyte, pushing an empty byte array on the stack), and should not push 0x00 on the stack (which is also interpreted as 0 in script.
- Specify that all tags 1 and above MUST be
OP_PUSHDATA_1 Nso that in the example of 1, the raw script would be 0x0101 (OP_PUSHDATA_1 + 0x01), and any value between -127 to 127 (excluding 0 and -0) is allowed. - However, parsers MUST allow for specific carve outs for semi-popular implementations that deviated, ie using OP_PUSHNUM_1 etc.
- The example should probably use OP names from Bitcoin Core, and instead of writing strings, write out the byte arrays with a comment afterward that shows the string.
That's about it I think... feel free to ignore, though, none of these are hills I'm willing to die on.
There was a problem hiding this comment.
I created #2505 to track this.
Thanks to @arik-so for pointing this out! see ordinals/ord#2497 see also ordinals/ord#2505
Now that we have the new envelope parser, this is very easy to do, so I thought why not. Finally recognize those early inscriptions which resulted from people manually creating inscriptions referring to our bad docs.
This would also allow us to save one byte per field ourselves by using a pushnum opcode instead of a pushbytes opcode.