Integration of property based testing into Bitcoin Core

Christewart commented at 11:39 pm on March 24, 2018: member

This PR is a subset of the changes in #8469. It’s meant to be easier to review. This PR contains all of the build instructions needed for travis to pass. It includes one property call key_properties.cpp along with a generator file called crypto_gen.{h,cpp}.

fanquake added the label Tests on Mar 24, 2018

practicalswift commented at 6:10 pm on March 25, 2018: contributor

Strong concept ACK

Very nice work!

randolf commented at 8:05 pm on March 25, 2018: contributor

@Christewart I noticed that depends/packages/rapidcheck.mk references MarcoFalke:

$(package)_download_path=https://github.com/MarcoFalke/rapidcheck/archive

Do you think it might be better to reference directly from “bitcoin” instead? If so, should MarcoFalke’s “rapidcheck” project be added to the “bitcoin” project to facilitate this?

randolf commented at 8:08 pm on March 25, 2018: contributor

@Christewart In src/test/key_properties.cpp there are new references to the BOOST libraries. As I understand it, one of the goals is to move away from BOOST, so I’m curious: Is it possible to accomplish the same goals in this code without the additional reliance on BOOST?

MarcoFalke requested review from theuni on Mar 25, 2018

MarcoFalke commented at 8:24 pm on March 25, 2018: member

We don’t plan to get rid of the boost unit test framework any time soon.

Christewart commented at 11:24 am on March 26, 2018: member

@randolf I’m not sure if it is too much of a concern right now who hosts the package. If we want to move it under the bitcoin project in the future i have no problem with that. Marco was kind enough to help me out with the setup which is why his repo is currently referenced.

MarcoFalke commented at 12:39 pm on March 26, 2018: member

@randolf Ideally it should reference the proper upstream, but since they don’t have any releases that might not work.

laanwj commented at 7:53 pm on March 27, 2018: member

We don’t plan to get rid of the boost unit test framework any time soon.

Indeed. @randolf In general: please don’t comment on boost usage of PRs in reviews if it concerns libraries that are already used in other parts of our code (such as boost test), only if it concerns using new boost libaries (such as lexical cast and whatnot.)

in src/test/gen/crypto_gen.cpp:4 in 900a441eea outdated

0@@ -0,0 +1,23 @@
1+#include "test/gen/crypto_gen.h"

fanquake commented at 8:14 am on April 19, 2018:

This and the .h are missing a copyright header:

0// Copyright (c) 2018 The Bitcoin Core developers
1// Distributed under the MIT software license, see the accompanying
2// file COPYING or http://www.opensource.org/licenses/mit-license.php.

in src/test/key_properties.cpp:3 in 900a441eea outdated

0@@ -0,0 +1,53 @@
1+// Copyright (c) 2012-2016 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.

fanquake commented at 8:15 am on April 19, 2018:

This can just be:

0// Copyright (c) 2018 The Bitcoin Core developers
1// Distributed under the MIT software license, see the accompanying
2// file COPYING or http://www.opensource.org/licenses/mit-license.php.

in depends/packages/rapidcheck.mk:8 in 900a441eea outdated

0@@ -0,0 +1,18 @@
1+package=rapidcheck
2+$(package)_version=10fc0cb
3+$(package)_download_path=https://github.com/MarcoFalke/rapidcheck/archive
4+$(package)_file_name=$(package)-$($(package)_version).tar.gz
5+$(package)_sha256_hash=9640926223c00af45bce4c7df8b756b5458a89b2ba74cfe3e404467f13ce26df
6+
7+define $(package)_config_cmds
8+  cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=true .

fanquake commented at 8:16 am on April 19, 2018:

Looks like we’ll need some documentation about installing/having cmake available

0Extracting rapidcheck...
1/Users/xxx/GitHub/bitcoin/depends/sources/rapidcheck-10fc0cb.tar.gz: OK
2Preprocessing rapidcheck...
3Configuring rapidcheck...
4/bin/sh: cmake: command not found

fanquake commented at 8:27 am on April 19, 2018: member

Testing https://github.com/bitcoin/bitcoin/pull/12775/commits/900a441eeacd9f936a2495e878174f9aeead66d2 on macOS 10.13.4:

After installing cmake I could successfully build rapidcheck in depends:

 0bash-3.2$ make NO_QT=1 RAPIDCHECK=1
 1Configuring rapidcheck...
 2-- The C compiler identification is AppleClang 9.1.0.9020039
 3-- The CXX compiler identification is AppleClang 9.1.0.9020039
 4-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc
 5-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -- works
 6-- Detecting C compiler ABI info
 7-- Detecting C compiler ABI info - done
 8-- Detecting C compile features
 9-- Detecting C compile features - done
10-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
11-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -- works
12-- Detecting CXX compiler ABI info
13-- Detecting CXX compiler ABI info - done
14-- Detecting CXX compile features
15-- Detecting CXX compile features - done
16-- Configuring done
17-- Generating done
18-- Build files have been written to: /Users/xxx/GitHub/bitcoin/depends/work/build/x86_64-apple-darwin17.5.0/rapidcheck/10fc0cb-3719dbaacbb
19Building rapidcheck...
20Scanning dependencies of target rapidcheck
21[  2%] Building CXX object CMakeFiles/rapidcheck.dir/src/BeforeMinimalTestCase.cpp.o
22[  5%] Building CXX object CMakeFiles/rapidcheck.dir/src/Check.cpp.o
23[  8%] Building CXX object CMakeFiles/rapidcheck.dir/src/Classify.cpp.o
24[ 11%] Building CXX object CMakeFiles/rapidcheck.dir/src/GenerationFailure.cpp.o
25[ 14%] Building CXX object CMakeFiles/rapidcheck.dir/src/Log.cpp.o
26[ 17%] Building CXX object CMakeFiles/rapidcheck.dir/src/Random.cpp.o
27[ 20%] Building CXX object CMakeFiles/rapidcheck.dir/src/Show.cpp.o
28[ 22%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Any.cpp.o
29[ 25%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Assertions.cpp.o
30[ 28%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Base64.cpp.o
31[ 31%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Configuration.cpp.o
32[ 34%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/DefaultTestListener.cpp.o
33[ 37%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/FrequencyMap.cpp.o
34[ 40%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/ImplicitParam.cpp.o
35[ 42%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/LogTestListener.cpp.o
36[ 45%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/MapParser.cpp.o
37[ 48%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/MulticastTestListener.cpp.o
38[ 51%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/ParseException.cpp.o
39[ 54%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Platform.cpp.o
40[ 57%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Property.cpp.o
41[ 60%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/PropertyContext.cpp.o
42[ 62%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/ReproduceListener.cpp.o
43[ 65%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Results.cpp.o
44[ 68%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Serialization.cpp.o
45[ 71%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/StringSerialization.cpp.o
46[ 74%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/TestMetadata.cpp.o
47[ 77%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/TestParams.cpp.o
48[ 80%] Building CXX object CMakeFiles/rapidcheck.dir/src/detail/Testing.cpp.o
49[ 82%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/Numeric.cpp.o
50[ 85%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/Text.cpp.o
51[ 88%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/ExecHandler.cpp.o
52[ 91%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/GenerationHandler.cpp.o
53[ 94%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/Recipe.cpp.o
54[ 97%] Building CXX object CMakeFiles/rapidcheck.dir/src/gen/detail/ScaleInteger.cpp.o
55[100%] Linking CXX static library librapidcheck.a
56[100%] Built target rapidcheck
57Staging rapidcheck...
58Postprocessing rapidcheck...
59Caching rapidcheck...

Did a ./configure --prefix=`pwd`/depends/x86_64-apple-darwin17.5.0:

0checking rapidcheck.h usability... yes
1checking rapidcheck.h presence... yes
2checking for rapidcheck.h... yes

You could add rapidcheck under the “Options used to compile and link:” at the end of ./configure

make check -j6 failed with:

 0Running tests: from test/gen/crypto_gen.cpp
 1Missing an argument value for the parameter run_test in the argument 
 2
 3Parameter: run_test
 4 Filters, which test units to include or exclude from test module execution.
 5 Command line formats:
 6   --run_test=<test unit filter>
 7   -t <test unit filter>
 8 Environment variable: BOOST_TEST_RUN_FILTERS
 9
10For detailed help on Boost.Test parameters use:
11  test_bitcoin --help
12or
13  test_bitcoin --help=<parameter name>
14make[3]: *** [test/gen/crypto_gen.cpp.test] Error 1
15make[3]: *** Waiting for unfinished jobs....
16make[2]: *** [check-am] Error 2
17make[1]: *** [check-recursive] Error 1
18make: *** [check-recursive] Error 1

fanquake added this to the milestone 0.17.0 on May 31, 2018

MarcoFalke commented at 11:55 am on July 14, 2018: member

Could you set RAPIDCHECK for the depends build and --with-rapidcheck for the configure, such that travis runs it?

Christewart force-pushed on Jul 14, 2018

Christewart force-pushed on Jul 15, 2018

Christewart commented at 0:20 am on July 15, 2018: member

@fanquake I fixed the issue with make check. I was accidentally classifying generator files as actual unit test files

Christewart force-pushed on Jul 15, 2018

Christewart commented at 1:23 pm on July 15, 2018: member

one build is still failing from a timeout issue it appears? I don’t think it is related to this pr?

MarcoFalke removed this from the milestone 0.17.0 on Jul 29, 2018

MarcoFalke added this to the milestone 0.18.0 on Jul 29, 2018

DrahtBot added the label Needs rebase on Aug 5, 2018

laanwj commented at 10:50 am on August 27, 2018: member

Probably a good time to move this forward and finally get it in now that 0.17 is branched off.

Integration of property based testing into Bitcoin Core

update copyright headers

attempt to fix linting errors

Fixing issue with make check classifying generator files as actual unit tests

Wrapping gen files in ENABLE_PROPERTY_TESTS macro

Make macro better

b2f49bd732

Christewart force-pushed on Aug 27, 2018

Christewart commented at 2:17 pm on August 27, 2018: member

@laanwj Rebased

DrahtBot removed the label Needs rebase on Aug 27, 2018

in configure.ac:1157 in b2f49bd732

1152+RAPIDCHECK_LIBS=
1153+if test "x$enable_property_tests" = xyes; then
1154+   RAPIDCHECK_LIBS=-lrapidcheck
1155+fi
1156+AC_SUBST(RAPIDCHECK_LIBS)
1157+AM_CONDITIONAL([ENABLE_PROPERTY_TESTS], [test x$enable_property_tests = xyes])

laanwj commented at 9:55 am on August 31, 2018:

I think something should be logged to the output depending on whether rapidcheck is used or not, currently there is nothing in configure output that suggests this

oh, just noticed in travis that without specifying explicit --with-rapidcheck there is:

0checking rapidcheck.h usability... no
1checking rapidcheck.h presence... no
2checking for rapidcheck.h... no

in depends/packages/rapidcheck.mk:13 in b2f49bd732

 8+  cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=true .
 9+endef
10+
11+define $(package)_build_cmds
12+  $(MAKE) && \
13+  mkdir -p $($(package)_staging_dir)$(host_prefix)/include && \

laanwj commented at 10:07 am on August 31, 2018:

indeed, this is necessary because rapidcheck has no make install

Christewart commented at 11:55 am on August 31, 2018:

This is an open PR on the repo

https://github.com/emil-e/rapidcheck/pull/152

Empact commented at 0:44 am on October 9, 2018:

Now that the PR is merged, should this be switched over?

laanwj commented at 10:09 am on August 31, 2018: member

I’m surprised that debian/ubuntu doesn’t package rapidcheck, the only way to get it is to build it from source.

laanwj commented at 10:14 am on August 31, 2018: member

This PR contains all of the build instructions needed for travis to pass.

it is not enabled in travis.yml though, is that intentional?

laanwj commented at 10:23 am on August 31, 2018: member

ACK

tested both without and with rapidcheck, and can confirm that the tests run in the latter case:

 0.../bitcoin/src/test/key_properties.cpp(22): Entering test suite "key_properties"
 1.../bitcoin/src/test/key_properties.cpp(25): Entering test case "key_uniqueness"
 2Using configuration: seed=9557370667331148370
 3.../bitcoin/src/test/key_properties.cpp(25): Leaving test case "key_uniqueness"; testing time: 3049us
 4.../bitcoin/src/test/key_properties.cpp(31): Entering test case "key_generates_correct_pubkey"
 5.../bitcoin/src/test/key_properties.cpp(31): Leaving test case "key_generates_correct_pubkey"; testing time: 36435us
 6.../bitcoin/src/test/key_properties.cpp(38): Entering test case "key_set_symmetry"
 7.../bitcoin/src/test/key_properties.cpp(38): Leaving test case "key_set_symmetry"; testing time: 2357us
 8.../bitcoin/src/test/key_properties.cpp(46): Entering test case "key_sign_symmetry"
 9.../bitcoin/src/test/key_properties.cpp(46): Leaving test case "key_sign_symmetry"; testing time: 28370us
10.../bitcoin/src/test/key_properties.cpp(22): Leaving test suite "key_properties"; testing time: 70316us

Christewart commented at 11:57 am on August 31, 2018: member

it is not enabled in travis.yml though, is that intentional?

Frankly I haven’t got that far. It’s not intentional, just extra work that I don’t have time to do ATM :-).

fanquake commented at 12:32 pm on August 31, 2018: member

@Christewart If you’re currently short on time, I can take this over.

On Fri, 31 Aug 2018 at 19:57, Chris Stewart notifications@github.com wrote:

it is not enabled in travis.yml though, is that intentional?

Frankly I haven’t got that far. It’s not intentional, just extra work that I don’t have time to do ATM :-).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/12775#issuecomment-417642035, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0t8vbkQrtybajNY4SDN2fDlhmUMWgNks5uWSSxgaJpZM4S58ml .

laanwj commented at 12:36 pm on August 31, 2018: member

Frankly I haven’t got that far. It’s not intentional, just extra work that I don’t have time to do ATM :-).

It’s fine, doesn’t have to be done in this PR. It’s just that your opening post mentions “This PR contains all of the build instructions needed for travis to pass.”

MarcoFalke commented at 3:04 pm on August 31, 2018: member

Enabling this on travis should just be setting RAPIDCHECK=1 in DEP_OPTS (and also installing cmake, i.e. add it to PACKAGES)

in src/test/gen/crypto_gen.cpp:14 in b2f49bd732

 9+#include <rapidcheck/Gen.h>
10+#include <rapidcheck/gen/Predicate.h>
11+#include <rapidcheck/gen/Container.h>
12+
13+/** Generates 1 to 20 keys for OP_CHECKMULTISIG */
14+rc::Gen<std::vector<CKey>> MultisigKeys()

practicalswift commented at 7:43 am on September 6, 2018:

MultisigKeys() seems unused? Is that intentional?

Christewart commented at 3:57 pm on September 6, 2018:

I don’t think it is used on this PR, but I believe it is used with the remaining test cases in #8469. Basically this PR splits #8469 in half – there is still a lot of test cases in that PR. Basically once this PR is merged i’m going to open another one with those test cases

Empact commented at 0:47 am on October 9, 2018:

I think we should be more careful about adding unused code like this under the expectation that it will be used in the future. YAGNI + concerns about maintaining an informative record in the code history.

laanwj deleted a comment on Sep 6, 2018

pull[bot] referenced this in commit eea87ef537 on Sep 8, 2018

laanwj merged this on Sep 8, 2018

laanwj closed this on Sep 8, 2018

in src/Makefile.test.include:11 in b2f49bd732

 7@@ -8,6 +8,7 @@ TEST_SRCDIR = test
 8 TEST_BINARY=test/test_bitcoin$(EXEEXT)
 9 
10 JSON_TEST_FILES = \
11+  test/data/script_tests.json \

MarcoFalke commented at 2:03 pm on September 9, 2018:

Why is this change required?

in depends/packages/rapidcheck.mk:17 in b2f49bd732

12+  $(MAKE) && \
13+  mkdir -p $($(package)_staging_dir)$(host_prefix)/include && \
14+  cp -a include/* $($(package)_staging_dir)$(host_prefix)/include/ && \
15+  cp -a extras/boost_test/include/rapidcheck/* $($(package)_staging_dir)$(host_prefix)/include/rapidcheck/ && \
16+  mkdir -p $($(package)_staging_dir)$(host_prefix)/lib && \
17+  cp -a librapidcheck.a $($(package)_staging_dir)$(host_prefix)/lib/

MarcoFalke commented at 6:02 pm on September 9, 2018:

Might want to update to the latest rapidcheck and use the make install target?

deadalnix referenced this in commit f9d5b98ef5 on Feb 27, 2020

deadalnix referenced this in commit 254e3daef8 on Feb 28, 2020

ftrader referenced this in commit 9613af3b3e on May 19, 2020

MarcoFalke locked this on Sep 8, 2021

Integration of property based testing into Bitcoin Core #12775