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

streams: Fix broken streams_vector_reader test. Remove unused seek(size_t). #14357

Merged

Conversation

Projects
None yet
5 participants
@practicalswift
Copy link
Member

commented Sep 30, 2018

Fix broken streams_vector_reader test. Remove unused seek(size_t).

Before this change the test streams_vector_reader triggered an unintended unsigned integer wraparound. It tried so seek using a negative value in reader.seek(-6).

Changes in this PR:

  • Fix broken VectorReader::seek(size_t) test case
  • Remove unused seek(size_t)

@practicalswift practicalswift force-pushed the practicalswift:vectorreader-seek-n-with-negative-n branch Sep 30, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

Friendly ping @jimpoVectorReader was introduced in 947133d and the test was added in 87f2d9e :-)

Should VectorReader::seek have SEEK_CUR or SEEK_SET behaviour? :-)

@practicalswift practicalswift changed the title Add support for negative relative seeks in VectorReader::seek (already assumed by tests) streams: Fix broken VectorReader::seek(n) + streams_vector_reader test Oct 1, 2018

@practicalswift practicalswift changed the title streams: Fix broken VectorReader::seek(n) + streams_vector_reader test streams: Fix broken VectorReader::seek(n) and/or streams_vector_reader test Oct 1, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

@donaloconnor Agreed! It could be that @jimpo meant to give seek absolute seek (SEEK_SET) behaviour (alternative 2 in the PR description) and the -6 (instead of 0) in the test is just a typo. With that solution the switch to signed integer is not needed :-)

I'll await some clarification on the intention here and adjust accordingly.

@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

Sorry I didn't mean to submit that. :-) just the first thing I spotted.

@jimpo
Copy link
Contributor

left a comment

Thanks, good catch. Concept ACK.

I copied seek from CVectorWriter, which is why it's a relative seek. If changing to SEEK_SET-type behavior, I think the same should be done for CVectorWriter.

Show resolved Hide resolved src/test/streams_tests.cpp Outdated
Show resolved Hide resolved src/test/streams_tests.cpp Outdated

@practicalswift practicalswift force-pushed the practicalswift:vectorreader-seek-n-with-negative-n branch 7 times, most recently Oct 3, 2018

@practicalswift practicalswift changed the title streams: Fix broken VectorReader::seek(n) and/or streams_vector_reader test streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust. Oct 3, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

@jimpo @donaloconnor PR updated. Please re-review :-)

@practicalswift practicalswift changed the title streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust. streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust. Add tests. Oct 3, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

Turns out that seek(…) is unused outside of the tests. Removed it.

Removed code is guaranteed to be bug free :-)

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

@jimpo Would you mind re-reviewing? :-)

@jimpo
Copy link
Contributor

left a comment

utACK 91f1848

Show resolved Hide resolved src/test/streams_tests.cpp Outdated
@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2018

Sorry I just realized I missed the new commit removing the seeks.

utACK 91f1848 👍

@practicalswift practicalswift force-pushed the practicalswift:vectorreader-seek-n-with-negative-n branch Oct 7, 2018

@practicalswift practicalswift changed the title streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust. Add tests. streams: Fix broken streams_vector_reader test. Remove unused seek(size_t). Oct 7, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2018

@jimpo @donaloconnor Feedback addressed. Please re-review :-)

@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

utACK ff7d6551feac15b60852b9d6553b54310800ccb6

@practicalswift practicalswift force-pushed the practicalswift:vectorreader-seek-n-with-negative-n branch Nov 6, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

Added commit 078155c8a2cc5eef5b92403e3d17c731d27b45d7 which removes UBSan suppression. Please re-review :-)

@jimpo

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

utACK 078155c8a2cc5eef5b92403e3d17c731d27b45d7

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

utACK 078155c

@practicalswift practicalswift force-pushed the practicalswift:vectorreader-seek-n-with-negative-n branch to 4f4993f Nov 23, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

Rebased!

@DrahtBot DrahtBot removed the Needs rebase label Nov 23, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

re-utACK 4f4993f

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2018

@jimpo @donaloconnor @meshcollider Would you mind reviewing/re-reviewing? :-)

@donaloconnor
Copy link
Contributor

left a comment

utACK - lgtm

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 5, 2019

Merge bitcoin#14357: streams: Fix broken streams_vector_reader test. …
…Remove unused seek(size_t).

4f4993f Remove UBSan suppression (practicalswift)
958e1a3 streams: Remove unused seek(size_t) (practicalswift)

Pull request description:

  Fix broken `streams_vector_reader` test. Remove unused `seek(size_t)`.

  Before this change the test `streams_vector_reader` triggered an unintended unsigned integer wraparound. It tried so seek using a negative value in `reader.seek(-6)`.

  Changes in this PR:
  * Fix broken `VectorReader::seek(size_t)` test case
  * Remove unused `seek(size_t)`

Tree-SHA512: 6c6affd680626363eef9e496748f2f86a522325abab9d6b13161f41125cdc29ceb36c2c1509c90b8ff108d606df7629e55e094cc2b6253b05a892b81ce176b71

@MarcoFalke MarcoFalke merged commit 4f4993f into bitcoin:master Jan 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

schancel pushed a commit to schancel/bitcoin-abc that referenced this pull request Apr 19, 2019

streams: Create VectorReader stream interface for vectors.
Summary:
This is a read analogue for the existing CVectorWriter.

For BIP158; is cherry-picked from two commits (947133d and 87f2d9e) from:
https://github.com/bitcoin/bitcoin/pull/12254/commits

In addition, the buggy seek() method was removed per later backport:
bitcoin/bitcoin#14357

Test Plan: `make check`

Reviewers: #bitcoin_abc, deadalnix, Fabien

Reviewed By: #bitcoin_abc, deadalnix, Fabien

Subscribers: Fabien

Maniphest Tasks: T589

Differential Revision: https://reviews.bitcoinabc.org/D2797

jonspock added a commit to jonspock/devault that referenced this pull request Apr 25, 2019

streams: Create VectorReader stream interface for vectors.
Summary:
This is a read analogue for the existing CVectorWriter.

For BIP158; is cherry-picked from two commits (947133d and 87f2d9e) from:
https://github.com/bitcoin/bitcoin/pull/12254/commits

In addition, the buggy seek() method was removed per later backport:
bitcoin/bitcoin#14357

Test Plan: `make check`

Reviewers: #bitcoin_abc, deadalnix, Fabien

Reviewed By: #bitcoin_abc, deadalnix, Fabien

Subscribers: Fabien

Maniphest Tasks: T589

Differential Revision: https://reviews.bitcoinabc.org/D2797

proteanx added a commit to devaultcrypto/devault that referenced this pull request Apr 26, 2019

streams: Create VectorReader stream interface for vectors.
Summary:
This is a read analogue for the existing CVectorWriter.

For BIP158; is cherry-picked from two commits (947133d and 87f2d9e) from:
https://github.com/bitcoin/bitcoin/pull/12254/commits

In addition, the buggy seek() method was removed per later backport:
bitcoin/bitcoin#14357

Test Plan: `make check`

Reviewers: #bitcoin_abc, deadalnix, Fabien

Reviewed By: #bitcoin_abc, deadalnix, Fabien

Subscribers: Fabien

Maniphest Tasks: T589

Differential Revision: https://reviews.bitcoinabc.org/D2797
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.