Use iterator in blockdata::script::Instructions
#673
Conversation
44b3dd4 to
b5d5e63
Compare
src/blockdata/script.rs
Outdated
| self.data.nth(data_len); // Kill iterator so that it does not return an infinite stream of errors | ||
| return Some(Err(Error::NonMinimalPush)); | ||
| } | ||
| let ret = Some(Ok(Instruction::PushBytes(&self.data.as_slice()[..n]))); |
There was a problem hiding this comment.
Ooops, n is not bound checked here because it relies on read_uint_iter which only checks it's len. Not caught by any test but thankfully fixed with improved design.
TODO write a test for this.
|
I pushed a greatly improved version, please take a look. |
|
@apoelstra addressed all your comments |
There was a problem hiding this comment.
I don't have much value to add by my review, just tried to understand what was going on.
EDIT: Oh, my minor suggestion got lost somehow during review. Here it is again, an alternate way to write one of the functions just in case you like it.
fn take_slice_or_kill(&mut self, len: usize) -> Result<&'a [u8], Error> {
if len == 0 {
return Ok(&[]);
}
if self.data.len() < len {
self.kill();
return Err(Error::EarlyEndOfScript);
}
let slice = &self.data.as_slice()[..len];
self.data.nth(len - 1); // Consume the sliced elements.
Ok(slice)
}
|
@tcharding if you do understand what's going on and you didn't find any problem that is very significant value: translates to ACK :) Your suggestion makes handling of zero more explicit but also pull is far away from the subtraction, so I'm not sure it's strictly better and frankly I'm not motivated to dismiss a review for it. But I noticed I could've written |
This refactors `blockdata::script::Instructions` to use `::core::slice::Iter<'a, u8>` instead of `&'a [u8]` to better express the intention and to avoid some slicing mistakes. Similarly to a previous change this uses a macro to deduplicate the common logic and the new `read_uint_iter` internal function to automatically advance the iterator. Addresses: rust-bitcoin#662 (review)
This creates a few primitive functions for handling iterators and uses them to avoid repeated code. As a result not only is the code simpler but also fixes a forgotten bound check. Thanks to a helper function which always does bounds check correctly this can no longer be forgotten.
* Changes `Option` to `Result` to avoid repeated `.ok_or(...)` * Renames `max` to `min_push_len` * Removes useless variable
Co-authored-by: Dr. Maxim Orlovsky <orlovsky@pandoracore.com>
af289df to
e6ff754
Compare
The code would've taken one element when an empty slice was requested. Co-authored-by: Tobin C. Harding <me@tobin.cc>
There was a problem hiding this comment.
ACK 2c28d3b. I don't want to hold ACKs on minor things as they can be in a fixup later.
| return Some(Err(Error::EarlyEndOfScript)); | ||
| } | ||
| if self.enforce_minimal { | ||
| if n == 1 && (self.data[1] == 0x81 || (self.data[1] > 0 && self.data[1] <= 16)) { | ||
| self.data = &[]; | ||
| // index acceess is safe because we checked the lenght above |
| Some(Ok(Instruction::PushBytes(&[]))) | ||
| }, | ||
| _ => { | ||
| Some(self.take_slice_or_kill(n).map(Instruction::PushBytes).ok_or(Error::EarlyEndOfScript)) |
There was a problem hiding this comment.
Note to self and other reviewers: There is no need to check enforce_minimal on 0 bytes as there is only one possible value (vec![])
| let data_len = self.data.len(); | ||
| self.data.nth(data_len); // Kill iterator so that it does not return an infinite stream of errors | ||
| return Some(Err(Error::NonMinimalPush)); | ||
| let op_byte = self.data.as_slice().first(); |
There was a problem hiding this comment.
nit:
This is not the op_byte. This is more like first_byte and we are checking first_byte is not equal to op_byte?
fn next(&mut self) -> Option<Result<Instruction<'a>, Error>> {
let &byte = self.data.next()?; // this should be op_byte|
OK, will try to make fixup PR. @tcharding "co-authored by" is inserted by GitHub, I don't mind. :) |
|
Merging this based on
|
…pt::Instructions` 2c28d3b Fix handling of empty slice in Instructions (Martin Habovštiak) e6ff754 Fix doc of take_slice_or_kill (Martin Habovštiak) 0ec6d96 Cleanup after `Instructions` refactoring (Martin Habovstiak) bc76325 Move repeated code to functions in script (Martin Habovstiak) 1f55edf Use iterator in `blockdata::script::Instructions` (Martin Habovstiak) Pull request description: This refactors `blockdata::script::Instructions` to use `::core::slice::Iter<'a, u8>` instead of `&'a [u8]` to better express the intention and to avoid some slicing mistakes. Similarly to a previous change this uses a macro to deduplicate the common logic and the new `read_uint_iter` internal function to automatically advance the iterator. Addresses: rust-bitcoin/rust-bitcoin#662 (review) ACKs for top commit: tcharding: ACK 2c28d3b sanket1729: ACK 2c28d3b. I don't want to hold ACKs on minor things as they can be in a fixup later. Tree-SHA512: 9dc770b9f7958efbd0df2cc2d3546e23deca5df2f94ea2c42b089df628f4b99f08032ca4aa8822caf6643a8892903e1bda41228b78c8519b90bcaa1255d9acc6
This refactors
blockdata::script::Instructionsto use::core::slice::Iter<'a, u8>instead of&'a [u8]to better expressthe intention and to avoid some slicing mistakes. Similarly to a
previous change this uses a macro to deduplicate the common logic and
the new
read_uint_iterinternal function to automatically advance theiterator.
Addresses:
#662 (review)