Use iterator in `blockdata::script::Instructions` by Kixunil · Pull Request #673 · rust-bitcoin/rust-bitcoin

Kixunil · 2021-10-02T18:34:49Z

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:
#662 (review)

src/blockdata/script.rs

dr-orlovsky

Found few nits. Concept ACK. Need more time for careful review and to ensure that all paths are test-covered.

src/blockdata/script.rs

Kixunil · 2021-10-07T14:59:32Z

src/blockdata/script.rs

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.

Kixunil · 2021-10-07T15:04:21Z

I pushed a greatly improved version, please take a look.

src/blockdata/script.rs

Kixunil · 2022-02-09T17:39:43Z

@apoelstra addressed all your comments

apoelstra

ACK 2349362b9fa38f660a3af016093157d768cd3b7a

tcharding

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)
    }

Kixunil · 2022-02-10T10:08:24Z

@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 saturating_sub(1) instead so will change it if there's another stronger reason to change the code.

dr-orlovsky

Catched one doc inconsistency with the code

src/blockdata/script.rs

apoelstra

ACK af289df56dd114ab6a29f41357583b366c07d764

dr-orlovsky

ACK af289df56dd114ab6a29f41357583b366c07d764, but needs rebase

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>

src/blockdata/script.rs

The code would've taken one element when an empty slice was requested. Co-authored-by: Tobin C. Harding <me@tobin.cc>

tcharding

ACK 2c28d3b

Thanks for the co-authored-by!

sanket1729

ACK 2c28d3b. I don't want to hold ACKs on minor things as they can be in a fixup later.

sanket1729 · 2022-04-21T22:19:52Z

src/blockdata/script.rs

                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


nit:
s/lenght/length

sanket1729 · 2022-04-21T22:43:00Z

src/blockdata/script.rs

+                        Some(Ok(Instruction::PushBytes(&[])))
+                    },
+                    _ => {
+                        Some(self.take_slice_or_kill(n).map(Instruction::PushBytes).ok_or(Error::EarlyEndOfScript))


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![])

sanket1729 · 2022-04-21T22:44:15Z

src/blockdata/script.rs

-                        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();


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

Kixunil · 2022-04-22T12:16:31Z

OK, will try to make fixup PR. @tcharding "co-authored by" is inserted by GitHub, I don't mind. :)

sanket1729 · 2022-04-30T22:24:56Z

Merging this based on

My and @tcharding's ACKs on tip.
@apoelstra's and @dr-orlovsky's previous ACKs and reviewing the minor range diff which was some whitspaces and the last commit.

…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

elichai reviewed Oct 2, 2021

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

Kixunil force-pushed the script_instructions_iter branch from 44b3dd4 to b5d5e63 Compare October 2, 2021 19:02

dr-orlovsky reviewed Oct 4, 2021

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

src/blockdata/script.rs Outdated Show resolved Hide resolved

src/blockdata/script.rs Outdated Show resolved Hide resolved

Kixunil commented Oct 7, 2021

View reviewed changes

Kixunil mentioned this pull request Oct 13, 2021

New Witness struct to improve ser/de perfomance #672

Merged

apoelstra reviewed Nov 12, 2021

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

apoelstra reviewed Nov 12, 2021

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

apoelstra reviewed Nov 12, 2021

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

apoelstra reviewed Nov 12, 2021

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

Kixunil added code quality Makes code easier to understand and less likely to lead to problems waiting for author This can only progress if the author responds to a request. labels Dec 2, 2021

apoelstra previously approved these changes Feb 9, 2022

View reviewed changes

Kixunil removed the waiting for author This can only progress if the author responds to a request. label Feb 9, 2022

tcharding reviewed Feb 10, 2022

View reviewed changes

dr-orlovsky reviewed Mar 12, 2022

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

Kixunil dismissed apoelstra’s stale review via af289df March 14, 2022 20:17

apoelstra previously approved these changes Mar 17, 2022

View reviewed changes

dr-orlovsky previously approved these changes Mar 18, 2022

View reviewed changes

Kixunil and others added 4 commits April 20, 2022 19:45

Cleanup after Instructions refactoring

0ec6d96

* Changes `Option` to `Result` to avoid repeated `.ok_or(...)` * Renames `max` to `min_push_len` * Removes useless variable

Fix doc of take_slice_or_kill

e6ff754

Co-authored-by: Dr. Maxim Orlovsky <orlovsky@pandoracore.com>

Kixunil dismissed stale reviews from dr-orlovsky and apoelstra via e6ff754 April 20, 2022 17:47

Kixunil force-pushed the script_instructions_iter branch from af289df to e6ff754 Compare April 20, 2022 17:47

Kixunil added the before edition bump We want to merge this before edition is bumped label Apr 20, 2022

tcharding reviewed Apr 20, 2022

View reviewed changes

src/blockdata/script.rs Show resolved Hide resolved

src/blockdata/script.rs Outdated Show resolved Hide resolved

src/blockdata/script.rs Show resolved Hide resolved

Fix handling of empty slice in Instructions

2c28d3b

The code would've taken one element when an empty slice was requested. Co-authored-by: Tobin C. Harding <me@tobin.cc>

tcharding approved these changes Apr 21, 2022

View reviewed changes

sanket1729 approved these changes Apr 21, 2022

View reviewed changes

Kixunil added this to the 0.29.0 milestone Apr 25, 2022

sanket1729 merged commit d5a28fc into rust-bitcoin:master Apr 30, 2022

Kixunil deleted the script_instructions_iter branch May 2, 2022 12:05

apoelstra mentioned this pull request Aug 16, 2025

Update bitcoin fuzz targets to use Arbitrary #4785

Merged

Conversation

Kixunil commented Oct 2, 2021

Uh oh!

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kixunil Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Oct 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kixunil commented Feb 9, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Feb 10, 2022

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Apr 22, 2022

Uh oh!

sanket1729 commented Apr 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tcharding left a comment •

edited

Loading

dr-orlovsky left a comment •

edited

Loading

sanket1729 Apr 21, 2022 •

edited

Loading