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
Deprecate script::read_uint
#1559
Deprecate script::read_uint
#1559
Conversation
Note to merger, may need to close #1547 manually . |
Damn, looks like we have to solve this ASAN problem. |
Can you rebase since ASAN is fixed? |
36ed20d
to
baa4a07
Compare
Rebased to pick up CI fix, no changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 36ed20d
There is no current usage for reading an unsigned script integer, seems like this is kruft from days gone past.
The URL is wrong (section `#Push_operators` should be `#push-operators`), also should use angle brackets not back ticks.
Our `Builder::push_int` method is the same as Bitcoin Core `CScript` `push_int64` method. We currently use `OP_FALSE` (equivalent to `OP_0`) but recently we added `OP_0`, lets use it to make our code better mimic Core (also saves devs checking that `OP_FALSE` is the same as `OP_0`).
We only support reads of upto 4 bytes where as Bitcoin Core allows reading a `CScriptNum` with more bytes than that. Add a rustdoc comment (incl. link to Bitcoin Core) mentioning that.
Our `script::read_scriptint` function is based on the constructor code (incl. call to `set_vch`) code from Bitcoin Core. Add rustdoc comment saying so, emit a link because there are already multiple links to `script.h` in this file (one just right below the added comment).
baa4a07
to
a7117bf
Compare
"rebased" manually (re-did the whole thing) and re-wrote PR description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a7117bf
@@ -167,6 +167,7 @@ pub fn read_scriptbool(v: &[u8]) -> bool { | |||
/// Note that this does **not** return an error for `size` between `core::size_of::<usize>()` | |||
/// and `u16::max_value / 8` if there's no overflow. | |||
#[inline] | |||
#[deprecated(since = "0.30.0", note = "bitcoin integers are signed 32 bits, use read_scriptint")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC this was meant only for reading push data length and got published by accident.
@@ -115,6 +115,8 @@ pub fn write_scriptint(out: &mut [u8; 8], n: i64) -> usize { | |||
/// don't fit in 64 bits (for efficiency on modern processors) so we | |||
/// simply say, anything in excess of 32 bits is no longer a number. | |||
/// This is basically a ranged type implementation. | |||
/// | |||
/// This code is based on the `CScriptNum` constructor in Bitcoin Core (see `script.h`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an implementation detail. I wouldn't use doc comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I'm not going to block the PR on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a7117bf
Patch one does the deprecation, the rest of the PR is made up of tiny improvements to the code around reading/writing 'scriptint's (conceptually
CScriptNum
s). I did all this while trying to decipher the discussion on #1547.Note Please
There are many more changes in the pipeline for all this read/write "script int" stuff. This PR was done ages ago and I believe it stall adds value.
I re-did the whole PR manually because of the recent
script
module changes. I hope no one else has to do that - if you do please feel free to holla and I'll "rebase" your PR for you.