Integration of property based testing into Bitcoin Core #12775

Christewart · 2018-03-24T23:39:49Z

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}.

practicalswift · 2018-03-25T18:10:26Z

Strong concept ACK

Very nice work!

randolf · 2018-03-25T20:05:46Z

@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 · 2018-03-25T20:08:52Z

@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?

maflcko · 2018-03-25T20:24:18Z

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

Christewart · 2018-03-26T11:24:47Z

@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.

maflcko · 2018-03-26T12:39:47Z

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

laanwj · 2018-03-27T19:53:08Z

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.)

fanquake · 2018-04-19T08:14:47Z

src/test/gen/crypto_gen.cpp

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


This and the .h are missing a copyright header:

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

fanquake · 2018-04-19T08:15:45Z

src/test/key_properties.cpp

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


This can just be:

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

fanquake · 2018-04-19T08:16:37Z

depends/packages/rapidcheck.mk

+$(package)_sha256_hash=9640926223c00af45bce4c7df8b756b5458a89b2ba74cfe3e404467f13ce26df
+
+define $(package)_config_cmds
+  cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=true .


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

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

fanquake · 2018-04-19T08:27:09Z

Testing 900a441 on macOS 10.13.4:

After installing cmake I could successfully build rapidcheck in depends:

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

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

checking rapidcheck.h usability... yes
checking rapidcheck.h presence... yes
checking 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:

Running tests: from test/gen/crypto_gen.cpp
Missing an argument value for the parameter run_test in the argument 

Parameter: run_test
 Filters, which test units to include or exclude from test module execution.
 Command line formats:
   --run_test=<test unit filter>
   -t <test unit filter>
 Environment variable: BOOST_TEST_RUN_FILTERS

For detailed help on Boost.Test parameters use:
  test_bitcoin --help
or
  test_bitcoin --help=<parameter name>
make[3]: *** [test/gen/crypto_gen.cpp.test] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [check-am] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [check-recursive] Error 1

maflcko · 2018-07-14T11:55:05Z

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

Christewart · 2018-07-15T00:20:17Z

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

Christewart · 2018-07-15T13:23:09Z

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

laanwj · 2018-08-27T10:50:12Z

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

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

Christewart · 2018-08-27T14:17:30Z

@laanwj Rebased

laanwj · 2018-08-31T09:55:09Z

configure.ac

+   RAPIDCHECK_LIBS=-lrapidcheck
+fi
+AC_SUBST(RAPIDCHECK_LIBS)
+AM_CONDITIONAL([ENABLE_PROPERTY_TESTS], [test x$enable_property_tests = xyes])


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:

checking rapidcheck.h usability... no checking rapidcheck.h presence... no checking for rapidcheck.h... no

laanwj · 2018-08-31T10:07:45Z

depends/packages/rapidcheck.mk

+
+define $(package)_build_cmds
+  $(MAKE) && \
+  mkdir -p $($(package)_staging_dir)$(host_prefix)/include && \


indeed, this is necessary because rapidcheck has no make install

This is an open PR on the repo

emil-e/rapidcheck#152

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

laanwj · 2018-08-31T10:09:27Z

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

laanwj · 2018-08-31T10:14:25Z

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 · 2018-08-31T10:23:06Z

ACK

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

.../bitcoin/src/test/key_properties.cpp(22): Entering test suite "key_properties"
.../bitcoin/src/test/key_properties.cpp(25): Entering test case "key_uniqueness"
Using configuration: seed=9557370667331148370
.../bitcoin/src/test/key_properties.cpp(25): Leaving test case "key_uniqueness"; testing time: 3049us
.../bitcoin/src/test/key_properties.cpp(31): Entering test case "key_generates_correct_pubkey"
.../bitcoin/src/test/key_properties.cpp(31): Leaving test case "key_generates_correct_pubkey"; testing time: 36435us
.../bitcoin/src/test/key_properties.cpp(38): Entering test case "key_set_symmetry"
.../bitcoin/src/test/key_properties.cpp(38): Leaving test case "key_set_symmetry"; testing time: 2357us
.../bitcoin/src/test/key_properties.cpp(46): Entering test case "key_sign_symmetry"
.../bitcoin/src/test/key_properties.cpp(46): Leaving test case "key_sign_symmetry"; testing time: 28370us
.../bitcoin/src/test/key_properties.cpp(22): Leaving test suite "key_properties"; testing time: 70316us

Christewart · 2018-08-31T11:57:26Z

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 · 2018-08-31T12:32:53Z

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

…

On Fri, 31 Aug 2018 at 19:57, Chris Stewart ***@***.***> 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 <#12775 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0t8vbkQrtybajNY4SDN2fDlhmUMWgNks5uWSSxgaJpZM4S58ml> .

laanwj · 2018-08-31T12:36:08Z

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."

maflcko · 2018-08-31T15:04:54Z

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

practicalswift · 2018-09-06T07:43:28Z

src/test/gen/crypto_gen.cpp

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


MultisigKeys() seems unused? Is that intentional?

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

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.

…in Core b2f49bd Integration of property based testing into Bitcoin Core (Chris Stewart) Pull request description: This PR is a subset of the changes in bitcoin#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}`. Tree-SHA512: 895c9d9273dcd29f696b1de8dfe1ee843095831bf1f68472844181278850bec36b20f0ba7e51e796112c5cc75cd24759f9f1771906503bbf3af16f627e18c6c9

maflcko · 2018-09-09T14:03:28Z

src/Makefile.test.include

@@ -8,6 +8,7 @@ TEST_SRCDIR = test
 TEST_BINARY=test/test_bitcoin$(EXEEXT)

 JSON_TEST_FILES = \
+  test/data/script_tests.json \


Why is this change required?

maflcko · 2018-09-09T18:02:07Z

depends/packages/rapidcheck.mk

+  cp -a include/* $($(package)_staging_dir)$(host_prefix)/include/ && \
+  cp -a extras/boost_test/include/rapidcheck/* $($(package)_staging_dir)$(host_prefix)/include/rapidcheck/ && \
+  mkdir -p $($(package)_staging_dir)$(host_prefix)/lib && \
+  cp -a librapidcheck.a $($(package)_staging_dir)$(host_prefix)/lib/


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

Summary: This is an initial integration of the rapidcheck property based tests. The original test has been slightly modified to adapt to our codebase. The feature has been added to CMake and the documentation updated accordingly. Note that as opposed to autotools, which enables the test automatically if `rapidcheck` is installed, it is disabled by default with cmake and requires a flag to be set by the user. Backport of core [[bitcoin/bitcoin#12775 | PR12775]], [[bitcoin/bitcoin#16622 | PR16622]] and [[bitcoin/bitcoin#16645 | PR16645]]. Test Plan: Read the doc and install rapidcheck. ../configure Ensure that rapidcheck is used by looking at the summary. make check Ensure there is no regression: cmake -GNinja .. ninja check cmake -GNinja .. -DENABLE_PROPERTY_BASED_TESTS=ON Check in the output that `rapidcheck` is enabled by searching for a couple lines similar to: ``` -- Found Rapidcheck: /usr/local/include -- Found Rapidcheck: /usr/local/lib/librapidcheck.a ``` ninja check Assuming you are running a 64-bit Linux machine: cd depends RAPIDCHECK=1 make build-linux64 Check the `rapidcheck` package is built. cd .. && mkdir buildLinux64 && cd buildLinux64 cmake -GNinja .. \ -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Linux64.cmake \ -DENABLE_PROPERTY_BASED_TESTS=ON Check the `rapidcheck` lib found is the one from the `depends`. ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5323

fanquake added the Tests label Mar 24, 2018

maflcko requested a review from theuni March 25, 2018 20:22

fanquake reviewed Apr 19, 2018

View reviewed changes

fanquake added this to the 0.17.0 milestone May 31, 2018

DrahtBot mentioned this pull request Jul 14, 2018

[depends] Don't build Qt dependencies if it doesn't support Qt #13660

Closed

Christewart force-pushed the rapidcheck_build branch 3 times, most recently from f90378c to a6df7f8 Compare July 15, 2018 00:19

Christewart force-pushed the rapidcheck_build branch 2 times, most recently from f9866ef to 4f7c64f Compare July 15, 2018 00:54

This was referenced Jul 22, 2018

[depends] Add riscv qt depends support for cross compiling bitcoin-qt #13710

Merged

Add aarch64 qt depends support for cross compiling bitcoin-qt #13696

Merged

[build] Add risc-v support to gitian #13665

Merged

Christewart mentioned this pull request Jul 24, 2018

Bugfixes for BIP 174 combining and deserialization #13721

Merged

maflcko modified the milestones: 0.17.0, 0.18.0 Jul 29, 2018

DrahtBot added the Needs rebase label Aug 5, 2018

Integration of property based testing into Bitcoin Core

b2f49bd

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

Christewart force-pushed the rapidcheck_build branch from 4f7c64f to b2f49bd Compare August 27, 2018 14:16

DrahtBot removed the Needs rebase label Aug 27, 2018

Christewart mentioned this pull request Aug 28, 2018

[POC] Introducing property based testing to Core #8469

Closed

laanwj reviewed Aug 31, 2018

View reviewed changes

practicalswift reviewed Sep 6, 2018

View reviewed changes

bitcoin deleted a comment Sep 6, 2018

laanwj merged commit b2f49bd into bitcoin:master Sep 8, 2018

maflcko reviewed Sep 9, 2018

View reviewed changes

Empact mentioned this pull request Oct 9, 2018

Add more property based tests for basic bitcoin data structures #14430

Closed

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

Empact mentioned this pull request Nov 8, 2018

tests: Separately check for rapidcheck/boost_test.h in configure #14694

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Integration of property based testing into Bitcoin Core #12775

Integration of property based testing into Bitcoin Core #12775

Uh oh!

Conversation

Christewart commented Mar 24, 2018

Uh oh!

practicalswift commented Mar 25, 2018

Uh oh!

randolf commented Mar 25, 2018

Uh oh!

randolf commented Mar 25, 2018

Uh oh!

maflcko commented Mar 25, 2018

Uh oh!

Christewart commented Mar 26, 2018

Uh oh!

maflcko commented Mar 26, 2018

Uh oh!

laanwj commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Apr 19, 2018

Uh oh!

maflcko commented Jul 14, 2018

Uh oh!

Christewart commented Jul 15, 2018

Uh oh!

Christewart commented Jul 15, 2018

Uh oh!

laanwj commented Aug 27, 2018

Uh oh!

Christewart commented Aug 27, 2018

Uh oh!

laanwj Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 31, 2018

Uh oh!

laanwj commented Aug 31, 2018

Uh oh!

laanwj commented Aug 31, 2018

Uh oh!

Christewart commented Aug 31, 2018

Uh oh!

fanquake commented Aug 31, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 31, 2018

Uh oh!

maflcko commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Christewart Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj Aug 31, 2018 •

edited

Loading

fanquake commented Aug 31, 2018 via email •

edited

Loading

Christewart Sep 6, 2018 •

edited

Loading