Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Apr 4, 2020

The current prevector test effectively randomly generates a number of operations to perform on a prevector and a normal vector, and checks consistency between the two.

By converting this into a fuzzer the operations can be targetted rather than random.

@fanquake fanquake added the Tests label Apr 4, 2020
@maflcko
Copy link
Member

maflcko commented Apr 5, 2020

The fuzz tests are not shipped on our website, nor are they run as part of make check. The existing unit test runs in a few milliseconds. So I'd slightly prefer to keep the unit tests for end users on odd compilers/platforms, which would otherwise be left standing in the rain. If you are worries about code duplication of the prevector_tester class, you may put it in ./src/test/util/, which is a library available to all tests.

Concept ACK on the new fuzzing harness.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ACK on the fuzzer

@sipa sipa force-pushed the 202004_prevector_fuzz branch from a8c3f23 to b1d24d1 Compare April 6, 2020 22:01
@sipa
Copy link
Member Author

sipa commented Apr 6, 2020

@MarcoFalke Rebased, addressed your comments, and removed removal of the existing unit test (I've duplicated instead of creating a common shared class, as I expect the two to diverge - the existing test is much more suited as an approach for fuzzing, while we may want a few static cases instead as unit tests).

@sipa sipa changed the title Convert randomized prevector test into fuzzer Add fuzzer version of randomized prevector test Apr 6, 2020
@maflcko
Copy link
Member

maflcko commented Apr 9, 2020

ACK b1d24d1 🍬

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK b1d24d1d031a2b2ce67bf846bafa1c3a499b7553 🍬
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjr+Qv/Xzpe1YkQG9BnrvlnlXx+mop66tdFHH3YXUWm8vaz9MPrQx3aWk1AW0xP
m8PcmwXxne6sl7ohdn4GKcM5LT1qbczArzQ409CRvyXsUOVXouuH0O2mVRxao2WE
ge+MivCWX9TUlieUBnXlb7Aa9BKprE8RlrCcpEPZ2mXtv/gtzjuzrt98kCJGOjRb
pN7cuwQegAXhWnEeZjzGW9WFzORN9aBQGWd9AxQT5ARt5zU2FPrDtRfNGLkIj0tF
0qCzmocq79NeMhugnSBGJMps66uGLjceUEd1jxntJUmjMJW/9LvNUS2IjjCu2GLW
aoogrsLtJGgAr/W/FOvvbFF5prgFEKhmtHPGcOIBEafbXHoS0AfVxlOOMekEijMF
YmTOs8mb2hOEa9K5tsFi1S5eCYZnH7ZIUF1fqzyiExbnLhuPKecgy1K0lLiOud1c
D+dk5L+9PU6u6X12QBl/w+j3b9kt8X+Cjse+UrZhiOWtBsicl/BKVxg45yzaH2BT
pHj6SgM2
=BYOH
-----END PGP SIGNATURE-----

@maflcko maflcko merged commit 405713d into bitcoin:master Apr 9, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2020
b1d24d1 Reorder the test instructions by number (Pieter Wuille)
c2ccadc Merge and generalize case 3 and case 6 (Pieter Wuille)
402ad5a Only run sanity check once at the end (Pieter Wuille)
eda8309 Assert immediately rather than caching failure (Pieter Wuille)
5560845 Make a fuzzer-based copy of the prevector randomized test (Pieter Wuille)

Pull request description:

  The current prevector test effectively randomly generates a number of operations to perform on a prevector and a normal vector, and checks consistency between the two.

  By converting this into a fuzzer the operations can be targetted rather than random.

ACKs for top commit:
  MarcoFalke:
    ACK b1d24d1 🍬

Tree-SHA512: 2b5c62abcd5fee94f42db03400531484d98c59e7f4308e0e683c61aabcd9ce42f85c5d058d2d5e7f8221124f71d2112b6a5f3c80e5d0fdae265a70647747e92f
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 21, 2021
Summary:
```
The current prevector test effectively randomly generates a number of
operations to perform on a prevector and a normal vector, and checks
consistency between the two.

By converting this into a fuzzer the operations can be targetted rather
than random.
```

Backport of core [[bitcoin/bitcoin#18529 | PR18529]].

Test Plan:
  ninja bitcoin-fuzzers
  ./src/test/fuzz/prevector <path_to_corpus>

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9003
test.resize(std::max(0, std::min(30, (int)test.size() + prov.ConsumeIntegralInRange<int>(0, 4) - 2)));
break;
case 2:
test.insert(prov.ConsumeIntegralInRange<size_t>(0, test.size()), 1 + prov.ConsumeBool(), prov.ConsumeIntegral<int>());
Copy link
Member

Choose a reason for hiding this comment

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

post-merge question: Is there a reason to limit the number of elements inserted in this case to 1 or 2?

kwvg added a commit to kwvg/dash that referenced this pull request Jul 6, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants