This is the second half of #12775.
In general property based testing is a great testing idiom for making sure invariants that you believe about your code hold true under a range of “valid” values.
This PR includes basic properties for
Currently rapidcheck is not enabled by default on travis – work has been done to enable in #14171 there appears to be a memory access violation in one of the environments.
0@@ -0,0 +1,58 @@
1+#ifndef BITCOIN_TEST_GEN_BLOCK_GEN_H
2+#define BITCOIN_TEST_GEN_BLOCK_GEN_H
3+
4+#include "test/gen/crypto_gen.h"
5+#include "test/gen/transaction_gen.h"
6+#include "uint256.h"
7+#include "primitives/block.h"
10@@ -14,6 +11,13 @@
11 rc::Gen<std::vector<CKey>> MultisigKeys()
12 {
13 return rc::gen::suchThat(rc::gen::arbitrary<std::vector<CKey>>(), [](const std::vector<CKey>& keys) {
14+ //TODO: Investigate why we can only allow 15 keys. Consensus rules
15+ // dictate we can up to 20 keys
16+ //https://github.com/bitcoin/bitcoin/blob/10bee0dd4f37eb6cb7a0f1d565fa0fecf8109c35/src/script/script.h#L29
17
Concept ACK
Very nice! Thanks for working on this
27+ uint256 hashPrevBlock;
28+ uint256 hashMerkleRoot;
29+ uint32_t nTime;
30+ uint32_t nBits;
31+ uint32_t nNonce;
32+ std::tie(nVersion, hashPrevBlock, hashMerkleRoot, nTime, nBits, nNonce) = primitives;
tie into the header fields, to make this more succinct.
19+ assert(result >= 0 && result < 1);
20+ return result;
21+ });
22+}
23+
24+rc::Gen<unsigned int> Between1And100()
nit: looks like this method is defined with the same name here and in merkleblock_gen.h, how about:
static more liberally?
static in merkleblock_gen.h and then include that header file and use it here?
bloom_gen.cpp implementation static will prevent it from being used outside this file, which would reduce the risk of conflicts and otherwise be appropriate as the method is only used here. I don’t think you want to apply static in the header, as that will result in the function being compiled into every including translation unit. Naming this to not have a potential conflict would also be positive.
7+#include <rapidcheck/Gen.h>
8+#include <rapidcheck/gen/Numeric.h>
9+#include <rapidcheck/gen/Container.h>
10+#include <rapidcheck/gen/Select.h>
11+
12+typedef std::pair<CScript, std::vector<CKey>> SPKCKeyPair;
using?
why gen::nonEmpty here?
I believe blocks, by consensus, are required to have at least one transaction in them (citation needed).
why gen::nonEmpty here?
I believe blocks, by consensus, are required to have at least one transaction in them (citation needed).
I believe that’s correct. Relevant check in CheckBlock here: https://github.com/bitcoin/bitcoin/blob/4de0b5f39cc35636d499ad29ee3c63384b13fc76/src/validation.cpp#L3112-L3118
46+ return std::make_pair(bloomFilter, hashes);
47+ });
48+}
49+
50+/** Loads an arbitrary bloom filter with the given hashes */
51+rc::Gen<std::pair<CBloomFilter, std::vector<uint256>>> LoadBloomFilter(const std::vector<uint256>& hashes)
Is the presence of LoadBloomFilter auto-detected?
Same question applies to LoadedBloomFilter, P2SHSPK, P2WSHSPK and SignedTx which all looks like unused functions when taking a naïve look :-)
0@@ -0,0 +1,34 @@
1+#include <boost/test/unit_test.hpp>
Needs copyright message :-)
Applies to all added files in this PR.
52+ block.vtx = refs;
53+ return block;
54+ });
55+ }
56+};
57+} //namespace rc
// namespace rc to make it consistent with the rest of the project.
Applies to all files with //namespace in this PR :-)
Rebooted the tests, as they were still stuck on the initial linting error.
This would be a good opportunity to address some of the nits left over from #12775, as well as possibly moving to a newer version of rapidcheck, which we can host at https://github.com/bitcoin-core. I bought that up on IRC with @laanwj here.
We should probably also pull in the depends documentation update from #14171.
FWIW I updated the PR description to point to #12775 instead of ##8469.
8+
9+#include <rapidcheck/gen/Arbitrary.h>
10+#include <rapidcheck/Gen.h>
11+
12+
13+typedef std::tuple<int32_t, uint256, uint256, uint32_t, uint32_t, uint32_t> BlockHeaderTup;
using instead :-)
10+
11+#include <rapidcheck/gen/Arbitrary.h>
12+#include <rapidcheck/Gen.h>
13+#include <rapidcheck/gen/Predicate.h>
14+
15+typedef std::tuple<const CTxOut, const CTransaction, const int> SpendingInfo;
using instead :-)
So I tried to adjust our build script that is found here and failed to sift through the cmake weeds.
It isn’t as trivial as running
0$ cmake .
1$ make install
since we needs extra functionality located in this package
https://github.com/bitcoin-core/rapidcheck/tree/master/extras
specifically the boost_test package. This is what is used to hook into the existing boost testing framework.
It would be much appreciated if someone who is more adept at cmake commenting about how to properly add that package in extras to have it properly installed in the rapidcheck.mk file.
@empact Yes! That is how I hook nicely into the existing boost testing framework.
What I am currently blocked on is adding that submodule to our ci build. Please see the comment above for details.
With these proeprties, if they are enabled, you can just run them simply with
0$ ./test_bitcoin
since we integrate with boost_test
EDIT:
Also I am short on time at the moment so I wont be able to work too much on getting the build stuff to work nicely
It looks like there is now an easy configuration option in rapidcheck to allow for the installing of extra/boost_test by setting a simple configuration option:
https://github.com/emil-e/rapidcheck/issues/222#issuecomment-443259105
We will need to update the rapidcheck dependency to at least https://github.com/emil-e/rapidcheck/commit/5f0eb302f28979395a070bbdfde1f6d4cad94409
@Christewart I’ve made an attempt to use the new install flags in #14853.
When I merge these changes on top of that PR (using the new rapidcheck) I’m seeing some compilation errors. This is on a macOS 10.14.1 machine, using Xcode 10.1 (10B61):
0In file included from test/gen/transaction_gen.cpp:1:
1In file included from ./test/gen/transaction_gen.h:4:
2In file included from ./test/gen/crypto_gen.h:4:
3In file included from ./key.h:10:
4In file included from ./pubkey.h:10:
5In file included from ./hash.h:11:
6In file included from ./crypto/sha256.h:10:
7In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:477:
8In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string_view:176:
9In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__string:56:
10In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/algorithm:643:
11In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:662:
12/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/tuple:227:10: error: static_assert failed due to requirement '__can_bind_reference<std::__1::vector<CTxIn, std::__1::allocator<CTxIn> > >()' "Attempted to construct a reference element in a tuple with an rvalue"
13 {static_assert(__can_bind_reference<_Tp>(),
14 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
15/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/tuple:411:15: note: in instantiation of function template specialization 'std::__1::__tuple_leaf<1, const std::__1::vector<CTxIn, std::__1::allocator<CTxIn> > &, false>::__tuple_leaf<std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, void>' requested here
16 : __tuple_leaf<_Indx, _Tp>(_VSTD::forward<typename tuple_element<_Indx,
17 ^
18/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/tuple:828:15: note: in instantiation of function template specialization 'std::__1::__tuple_impl<std::__1::__tuple_indices<0, 1, 2, 3>, int, const std::__1::vector<CTxIn, std::__1::allocator<CTxIn> > &, const std::__1::vector<CTxOut, std::__1::allocator<CTxOut> > &, unsigned int>::__tuple_impl<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, void>' requested here
19 : __base_(_VSTD::forward<_Tuple>(__t)) {}
20 ^
21/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/shrinkable/Transform.hpp:20:37: note: in instantiation of function template specialization 'std::__1::tuple<int, const std::__1::vector<CTxIn, std::__1::allocator<CTxIn> > &, const std::__1::vector<CTxOut, std::__1::allocator<CTxOut> > &, unsigned int>::tuple<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, false>' requested here
22 U value() const { return m_mapper(m_shrinkable.value()); }
23 ^
24/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/Shrinkable.hpp:26:44: note: in instantiation of member function 'rc::shrinkable::detail::MapShrinkable<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)>::value' requested here
25 T value() const override { return m_impl.value(); }
26 ^
27/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/Shrinkable.hpp:22:12: note: in instantiation of member function 'rc::Shrinkable<CTransaction>::ShrinkableImpl<rc::shrinkable::detail::MapShrinkable<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)> >::value' requested here
28 explicit ShrinkableImpl(Args &&... args)
29 ^
30/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/Shrinkable.hpp:101:27: note: (skipping 3 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
31 shrinkable.m_impl = new ShrinkableImpl(std::forward<Args>(args)...);
32 ^
33/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/Gen.hpp:39:12: note: in instantiation of member function 'rc::gen::detail::MapGen<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)>::operator()' requested here
34 return m_impl(random, size);
35 ^
36/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/Gen.hpp:34:3: note: in instantiation of member function 'rc::Gen<CTransaction>::GenImpl<rc::gen::detail::MapGen<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)> >::generate' requested here
37 GenImpl(Args &&... args)
38 ^
39/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/Gen.hpp:58:18: note: in instantiation of function template specialization 'rc::Gen<CTransaction>::GenImpl<rc::gen::detail::MapGen<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)> >::GenImpl<rc::gen::detail::MapGen<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)> >' requested here
40 : m_impl(new GenImpl<Decay<Impl>>(std::forward<Impl>(impl))) {}
41 ^
42/Users/michael/github/bitcoin/depends/x86_64-apple-darwin18.2.0/share/../include/rapidcheck/gen/Transform.hpp:78:10: note: in instantiation of function template specialization 'rc::Gen<CTransaction>::Gen<rc::gen::detail::MapGen<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)>, void>' requested here
43 return detail::MapGen<T, Decay<Mapper>>(std::move(gen),
44 ^
45./test/gen/transaction_gen.h:95:21: note: in instantiation of function template specialization 'rc::gen::map<std::__1::tuple<int, std::__1::vector<CTxIn, std::__1::allocator<CTxIn> >, std::__1::vector<CTxOut, std::__1::allocator<CTxOut> >, unsigned int>, (lambda at ./test/gen/transaction_gen.h:97:13)>' requested here
46 return gen::map(gen::tuple(gen::arbitrary<int32_t>(),
The entire build output is available here.
Rebased this branch, but i am getting a new error when running these tests locally. This doesn’t seem related to my PR AFAICT
0chris@chris:~/dev/bitcoin/src/test$ ./test_bitcoin
1Running 365 test cases...
2Using configuration: seed=16472686160129483926
3Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1555532449_497584857/tempdir/path_does_not_exist" does not exist
4Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1555532449_512688440/tempdir/not_a_directory.dat" is not a directory
5Error: Specified -walletdir "wallets" is a relative path
0@@ -0,0 +1,35 @@
1+// Copyright (c) 2012-2016 The Bitcoin Core developers
0@@ -1,6 +1,3 @@
1-// Copyright (c) 2018 The Bitcoin Core developers
2-// Distributed under the MIT software license, see the accompanying
3-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 #include <test/gen/crypto_gen.h>
.h.
@Christewart Do you have time to continue with this RapidCheck test work going forward? If so, could you rebase this on master (to pull in a build fix), address outstanding nits and squash your commits etc.
I checked this out and ran the tests. For some reason I thought I had (or used to have?) rapidcheck installed via brew, but it doesn’t seem to be available from there at the moment.
I’m wondering what the state of the RapidCheck project is, as the repo hasn’t seen any activity for 6 months, and the author seems to have gone quiet on GitHub. There are a few issues as well as PRs piling up on that repo.
So i rebased onto master, and fixed various things that needed to be fixed with API changes in master.
Do you have time to continue with this RapidCheck test work going forward? If so, could you rebase this on master (to pull in a build fix), address outstanding nits and squash your commits etc.
I’m wondering what the state of the RapidCheck project is, as the repo hasn’t seen any activity for 6 months, and the author seems to have gone quiet on GitHub. There are a few issues as well as PRs piling up on that repo.
I think this is concerning as well, I do not have the time or know how to maintain rapidcheck itself. I see you have an issue on the repo about keeping rapidcheck working with newer versions of gcc, and it hasn’t been answered which is concerning.
I checked this out and ran the tests. For some reason I thought I had (or used to have?) rapidcheck installed via brew, but it doesn’t seem to be available from there at the moment.
I don’t think rapidcheck can be installed with brew (admittedly, I don’t have a mac). I think you need to do
0rapidcheck $ cmake . && make install
1-- Configuring done
2-- Generating done
3-- Build files have been written to: /home/chris/dev/rapidcheck
4[100%] Built target rapidcheck
5Install the project...
6-- Install configuration: ""
7-- Up-to-date: /usr/local/lib/librapidcheck.a
8-- Up-to-date: /usr/local/include
9-- Up-to-date: /usr/local/include/rapidcheck.h
10-- Up-to-date: /usr/local/include/rapidcheck
11....
12-- Up-to-date: /usr/local/include
13-- Up-to-date: /usr/local/include/rapidcheck
14-- Up-to-date: /usr/local/include/rapidcheck/boost_test.h
152@@ -153,11 +153,25 @@ BITCOIN_TESTS =\
153
154 if ENABLE_PROPERTY_TESTS
155 BITCOIN_TESTS += \
156- test/key_properties.cpp
157+ test/block_properties.cpp \
Concept ACK, approach ACK. Built and ran all tests successfully, both this branch and also rebased on latest master, with Linux Debian and a fresh build of RapidCheck on master at https://github.com/emil-e/rapidcheck/commit/d9482c683429fe79122e3dcab14c9655874aeb8e.
0 CXX test/gen/test_bitcoin-block_gen.o
1 CXX test/gen/test_bitcoin-bloom_gen.o
2 CXX test/gen/test_bitcoin-crypto_gen.o
3 CXX test/gen/test_bitcoin-script_gen.o
4 CXX test/gen/test_bitcoin-transaction_gen.o
5
6[...]
7
8 CXX test/test_bitcoin-block_properties.o
9 CXX test/test_bitcoin-bloom_properties.o
10 CXX test/test_bitcoin-key_properties.o
11 CXX test/test_bitcoin-merkleblock_properties.o
12 CXX test/test_bitcoin-script_properties.o
13 CXX test/test_bitcoin-transaction_properties.o
14
15[...]
16
17Running tests: block_properties from test/block_properties.cpp
18Running tests: bloom_properties from test/bloom_properties.cpp
19Running tests: key_properties from test/key_properties.cpp
20Running tests: merkleblock_properties from test/merkleblock_properties.cpp
21Running tests: script_properties from test/script_properties.cpp
22Running tests: transaction_properties from test/transaction_properties.cpp
@Christewart could you put this PR into final ACK-ready form? I’m guessing multiple commits for the different tests or Makefile changes would probably be fine (given the work put into it) but would be good to squash the linter, typo, rebase, and clang commits.
Rapidcheck build aspects could be addressed in a separate PR as this one concentrates on adding tests.
Note that Rapidcheck has recently seen a bit of renewed maintainer activity, though afaik https://github.com/emil-e/rapidcheck/issues/232 filed by @fanquake hasn’t been addressed yet.
fix typo in Makefile.test.include
rebase and redo imports
Rebase onto master, fix compile erros with various API changes in master, address nits
Run clang-format-diff.py
Add missing copyright header