Correctly parse inscriptions when using minimal opcodes

[arik-so]

Currently, minimal opcodes like OP_1 and OP_2 don't get recognized because all opcode operations that aren't OP_ENDIF result in an invalid inscription error, necessitating that all numeric opcodes be pushed as slices instead. However, that is in violation of BIP 62 motivation §3.

[tyjvazum]

@arik-so, could you provide a short description of the logic used in your changes?

[arik-so]

Certainly! In the code, or as a comment on the PR?

The point is basically that the documentation suggests using OP_1, OP_2, etc. for the various content property tags. However, the way it's currently being handled, OP_1 would not actually be recognized, because it would match the Instruction::Op arm. The issue is that rust-bitcoin's push_slice does not do minimal opcode detection, so pushing &[1] actually results in the hex encoding of 0101 instead of the opcode hex encoding 51 (81 in decimal). The actual opcode would be interpreted as invalid.

The set of numeric opcodes ranges from 1-16 (it just so happens that OP_0 and OP_FALSE and the 0-byte-slice are all encoded identically as (00), so that does not cause any breakages.

[tyjvazum]

Thank you for the detailed, clear explanation. I don't think code comments are necessary. However, I do have a couple other questions: what's the logic for using Vec instead of u8, and shouldn't this same change be made (for building as well as parsing) in the append_reveal_script_to_builder function, which currently uses push_slice?

[arik-so]

To answer your first question, the reason I had to make it a Vec instead of a slice is that in the ::Op case, I'm instantiating a new vector, and so merely using a reference would result in the vector getting dropped with the closure of the match arm, a problem which the match arm that receives a reference to a vec with a longer lifetime doesn't have.

Regarding your other question, I don't think append_reveal_script_to_builder should be modified yet, because people running older versions of ord would then not see inscriptions generated after this change gets merged. I think that change should happen in due time, maybe a couple weeks or a month after the parsing becomes more permissive. Or perhaps even sooner if the software updates propagate more quickly.

[shesek]

Couldn't this be solved by using Builder::push_int() instead of Builder::push_slice() for pushing the tags and Script::instructions_minimal() instead of Script::instructions()?

[arik-so]

That would fix the script construction, but not its parsing. As I pointed out in my previous comment, the construction should not be modified until the parsing fix propagates to most ord clients such that the inscriptions don't break.

It's a followup PR that I hope to make in a couple weeks or so, if/when this one gets merged.

[shesek]

Why won't instructions_minimal() fix parsing?

[arik-so]

Sorry, instructions_minimal would improve parsing (though it obviously still requires the ::Ops change), but I strongly believe that the parsing fix should go out several weeks prior to the inscription change. I think though that even with instructions_minimal, the slice match arm of the parser would remain necessary for any tags above 16.

[brandonblack]

Ping! Relaxing this parsing in ord seems important for future interoperability, even though it will renumber inscriptions.

[arik-so]

Any chance CI might be permitted to run on this PR?

[tyjvazum]

@arik-so, I'm working on arb. Any improvements you have in mind would be very welcome 👍

[veryordinally]

Any chance CI might be permitted to run on this PR?

@arik-so if you could update this PR we'll run CI. We'd like to integrate this for completing the cursed inscriptions handling.

[veryordinally]

The code to recognize and parse the inscriptions with minimal opcodes looks good to me.

We would need to make these inscriptions cursed, however, as otherwise existing inscription numbers will change. So we need a new type of curse, and then need to percolate a cursed status up from where we handle the minimal opcodes.

A full test would entail comparing an index generated with this PR with a previous index and verifying that all positive inscription numbers remain unchanged (index can be exported with index export into an easily comparable format).

Let me know if you feel like this is something you'd be up to doing, or if I should add it to my backlog.

[arik-so]

Probably not until this upcoming weekend. If you have pointers towards similar unit tests though, they'd be greatly appreciated! Thanks!

[veryordinally]

Probably not until this upcoming weekend. If you have pointers towards similar unit tests though, they'd be greatly appreciated! Thanks!

This would be a test on the full mainnet chain so outside our unit test and integration test framework. I also won't get to this before the weekend, happy to collaborate on this then.

[arik-so]

@veryordinally I added cursing to the inscription, but no external curse verification yet.

[veryordinally]

Looks good - will format and resolve conflicts, then verify inscription number correctness.

[veryordinally]

@arik-so Sorry this took me a while to get to. Reviewed the cursed changes, looks good so far. I fixed the formatting so our CI is happy, but noticed your two tests are failing. Could you look into that?

[veryordinally]

@arik-so Looked like the tests weren't updated for the cursed logic - please check if my changes make sense. Will now run inscription number validation, which will take a day or so.

[arik-so]

You are right, they weren't. I didn't make the cursing boolean setting public in the constructor, so the tests for verifying the newly created inscriptions had mismatched internal values for the structs, and time got away from me so I never got around to fixing that. Taking a look at your changes now, thanks!

[arik-so]

Your tests look great. I am personally still not super happy about the uses_minimal_opcodes field just hanging out at the top level of the Inscription struct, but I felt like nesting it deeper would result in even more technical debt down the line, though who knows.

[veryordinally]

See also #1403

[arik-so]

Sorry, been very busy. Will look into the performance penalty issue.

[arik-so]

Hey @veryordinally, I have modified the pattern matching to only match on the positive push num operators. I suspect having to run op_code.to_u8() on every single op code may have been a contributing factor to the slowdown.

[raphjaph]

We merged this: #2497