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

Feature: Instruction can read the script number #2081

Merged

Conversation

junderw
Copy link
Contributor

@junderw junderw commented Sep 18, 2023

This new method on Instruction will allow for interpreting the Instruction as a script number (i64)

@apoelstra
Copy link
Member

Looks good! But we have an existing read_scriptint function that I think can do the PushBytes case. (Or did you check that it wasn't applicable for some reason?)

Could you also add a unit test for a couple negative zeros :}?

@junderw junderw force-pushed the junderw/instruction-scriptnum branch 4 times, most recently from 6b2062c to c7ff61d Compare September 19, 2023 06:22
@apoelstra
Copy link
Member

Ok, c7ff61d looks good now! But what do you think of renaming read_scriptint_internal to read_scriptint_nonminimal?

@junderw
Copy link
Contributor Author

junderw commented Sep 19, 2023

Should I expose it publicly?

In which case, we'd still want the > 4 byte check for both.

@apoelstra
Copy link
Member

@junderw yeah, I think it should be exposed publicly.

@junderw junderw force-pushed the junderw/instruction-scriptnum branch 2 times, most recently from 4dffe7c to 3bdef42 Compare September 19, 2023 14:24
@junderw
Copy link
Contributor Author

junderw commented Sep 19, 2023

Should be good for re-review now.

apoelstra
apoelstra previously approved these changes Sep 19, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3bdef42

bitcoin/src/blockdata/script/mod.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/script/mod.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/script/instruction.rs Show resolved Hide resolved
bitcoin/src/blockdata/script/instruction.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/script/instruction.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/script/mod.rs Show resolved Hide resolved
bitcoin/src/blockdata/script/push_bytes.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/script/tests.rs Show resolved Hide resolved
@junderw
Copy link
Contributor Author

junderw commented Sep 20, 2023

If tests pass, this is ready for re-review

@tcharding
Copy link
Member

tcharding commented Sep 20, 2023

Ouch, CI fail is linter because if v.len() == 0 should be if v.is_empty().

Scond time this week my review suggestions have failed to build, bad robot.

tcharding
tcharding previously approved these changes Sep 20, 2023
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 767b94e

Thanks for working in my suggestions, appreciate the extra effort.

bitcoin/src/blockdata/script/mod.rs Show resolved Hide resolved
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cd15c74

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cd15c74

@apoelstra apoelstra merged commit f5b1788 into rust-bitcoin:master Sep 20, 2023
29 checks passed
@stevenroose
Copy link
Collaborator

Nice, this is useful!

Scond time this week my review suggestions have failed to build, bad robot.

You mean lint* not build ;)

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

Successfully merging this pull request may close these issues.

None yet

4 participants