Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup opcode classification #408

Open
wants to merge 2 commits into
base: master
from

Conversation

@murtyjones
Copy link

murtyjones commented Feb 6, 2020

This PR cleans up up the encapsulation of classify() in opcode.rs, and adds testing around the method for each classification.

murtyjones added 2 commits Feb 6, 2020
is_in_class!(OP_PUSHNUM_NEG1, C::PushNum(-1));

is_in_class!(
OP_PUSHNUM_1,

This comment has been minimized.

Copy link
@stevenroose

stevenroose Feb 7, 2020

Collaborator

Just a quick question, what happens to 0?

This comment has been minimized.

Copy link
@murtyjones

murtyjones Feb 8, 2020

Author

not sure i'm following as there is no OP_PUSHNUM_0

This comment has been minimized.

Copy link
@apoelstra

apoelstra Feb 9, 2020

Member

PUSHBYTES_0 is the same as PUSHNUM_0 for all intents and purposes, and it will be very confusing if the numeric opcodes include -1 through 16 but not 0.

This comment has been minimized.

Copy link
@murtyjones

murtyjones Feb 9, 2020

Author

Makes sense. I'm still learning basics of the protocol, but is it the solution to simply add OP_PUSHNUM_0 here and have it return Class::PushNum(0)?

This comment has been minimized.

Copy link
@apoelstra

apoelstra Feb 10, 2020

Member

No, because it is also the 0-byte push opcode :)

This comment has been minimized.

Copy link
@murtyjones

murtyjones Feb 10, 2020

Author

Sorry, I'm really not following. What would be the solution here?

This comment has been minimized.

Copy link
@apoelstra

apoelstra Feb 10, 2020

Member

I guess, we should leave the code as-is since that matches the existing logic. But IMO this is a wart in the library that I'd really rather fix.

On the other hand, there isn't really any good solution. The fact is that not all opcodes can be classified uniquely. (We have a similar problem actually with all the numeric opcodes, which are equivalent to pushdata opcodes but classified differently ... and regardless of that, in many contexts numerics should be considered as pushes.)

return Class::PushNum(-1);
} else if self.is_push_num_positive() {
// 16 opcodes
return Class::PushNum(1 + self.code as i32 - all::OP_PUSHNUM_1.code as i32);

This comment has been minimized.

Copy link
@stevenroose

stevenroose Feb 7, 2020

Collaborator

Just a quick question, what happens to 0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.