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.
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.
218 | + } 219 | + test.insert_range(prov.ConsumeIntegralInRange<size_t>(0, test.size()), values, values + num); 220 | + break; 221 | + } 222 | + case 6: { 223 | + int del = std::min<int>(test.size(), 1 + prov.ConsumeIntegralInRange<int>(0, 3));
Could this be combined with case 3 and generalized?
int del = prov.ConsumeIntegralInRange<int>(1, test.size());
Indeed, done (and the del range can start at 0 even).
25 | + pretype pre_vector_alt; 26 | + 27 | + typedef typename pretype::size_type Size; 28 | + 29 | +public: 30 | + void test() {
Can the test method be made read-only? If yes, I think case 15 can be removed?
void test() const {
Right, my thinking was that the fuzzer should be allowed to find intermediate failures too, but I think that's pointless. A shorter fuzzer input (which runs test once at the end) would accomplish the same.
Done.
ACK on the fuzzer
49 | + } 50 | + for (const T& v : const_pre_vector) { 51 | + assert(v == real_vector[pos++]); 52 | + } 53 | + for (const T& v : reverse_iterate(const_pre_vector)) { 54 | + assert(v == real_vector[--pos]);
I think the assert-side-effects linter complains about this, so you might have two options:
Assertions should not have side effects:
src/test/fuzz/prevector.cpp: assert(v == real_vector[pos++]);
src/test/fuzz/prevector.cpp: assert(v == real_vector[--pos]);
src/test/fuzz/prevector.cpp: assert(v == real_vector[pos++]);
src/test/fuzz/prevector.cpp: assert(v == real_vector[--pos]);
^---- failure generated from test/lint/lint-assertions.sh
Fixed by removing the side effect.
622 | @@ -623,6 +623,12 @@ test_fuzz_parse_univalue_LDADD = $(FUZZ_SUITE_LD_COMMON) 623 | test_fuzz_parse_univalue_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) 624 | test_fuzz_parse_univalue_SOURCES = $(FUZZ_SUITE) test/fuzz/parse_univalue.cpp 625 | 626 | +test_fuzz_prevector_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) 627 | +test_fuzz_prevector_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) 628 | +test_fuzz_prevector_LDADD = $(FUZZ_SUITE_LD_COMMON) 629 | +test_fuzz_prevector_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) 630 | +test_fuzz_prevector_SOURCES = $(FUZZ_SUITE) test/fuzz/prevector.cpp
Needs rebase, sorry
test_fuzz_prevector_SOURCES = test/fuzz/prevector.cpp
No worries, done.
@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).
ACK b1d24d1d031a2b2ce67bf846bafa1c3a499b7553 🍬
<details><summary>Show signature and timestamp</summary>
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-----
</details>
195 | + break; 196 | + case 1: 197 | + test.resize(std::max(0, std::min(30, (int)test.size() + prov.ConsumeIntegralInRange<int>(0, 4) - 2))); 198 | + break; 199 | + case 2: 200 | + test.insert(prov.ConsumeIntegralInRange<size_t>(0, test.size()), 1 + prov.ConsumeBool(), prov.ConsumeIntegral<int>());
post-merge question: Is there a reason to limit the number of elements inserted in this case to 1 or 2?