Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POC] Introducing property based testing to Core #8469

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@Christewart
Copy link
Contributor

commented Aug 6, 2016

This pull request is a proof of concept for introducting property based testing into Bitcoin Core

In QuickCheck the programmer writes assertions about logical properties that a function should fulfill. Then QuickCheck attempts to generate a test case that falsifies these assertions. Once such a test case is found, QuickCheck tries to reduce it to a minimal failing subset by removing or simplifying input data that are not needed to make the test fail.

This has been very useful for a Bitcoin library I've been working on and thought it would be worthwhile to develop a POC for Bitcoin Core. The property based library I am using for C++ is called rapidcheck. Here are the docs.

This pull request currently contains two properties, one testing CKey generation and the other testing serialization symmetry for CKey and CBitcoinSecret. These are rather trivial properties, but useful for illustrating the power of property based testing if there was a bug inside of Core.

I want to solicit some feedback from developers if this is something that would actually be merged into Core. Eventually we could have a large library of generators that would allow us to quickly prototype, test, and reason about the behavior of new code added to Core. Here is an example of a library of generators (in Scala) that could give you a little more of an idea of what I am talking about.

Thoughts?

@fanquake

View changes

depends/packages/rapidcheck.mk Outdated

$(package)_download_path:https://github.com/Christewart/rapidcheck/releases/download/1.0

$(package)_file_name:rapidcheck-1.0.tar.gz

This comment has been minimized.

Copy link
@fanquake

fanquake Aug 8, 2016

Member

You should be able to use something like $(package)-$($(package)_version) here.

@fanquake

View changes

depends/packages/rapidcheck.mk Outdated
@@ -0,0 +1,21 @@
package=rapidcheck

This comment has been minimized.

Copy link
@fanquake

fanquake Aug 8, 2016

Member

No need for the empty lines.

@fanquake

View changes

depends/packages/rapidcheck.mk Outdated
@@ -0,0 +1,21 @@
package=rapidcheck

$(package)_version:1.0

This comment has been minimized.

Copy link
@fanquake

fanquake Aug 8, 2016

Member

Use = instead of : here, as well as the lines below.

@fanquake

View changes

configure.ac Outdated
@@ -508,7 +508,7 @@ if test x$TARGET_OS = xdarwin; then
AX_CHECK_LINK_FLAG([[-Wl,-dead_strip]], [LDFLAGS="$LDFLAGS -Wl,-dead_strip"])
fi

AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h stdio.h stdlib.h unistd.h strings.h sys/types.h sys/stat.h sys/select.h sys/prctl.h])
AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h stdio.h stdlib.h unistd.h strings.h sys/types.h sys/stat.h sys/select.h sys/prctl.h ])

This comment has been minimized.

Copy link
@fanquake

fanquake Aug 8, 2016

Member

nit: space introduced here.

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

This needs a rebase/conflicts fixed.

@fanquake

View changes

src/test/key_properties.cpp Outdated
@@ -0,0 +1,48 @@
// Copyright (c) 2012-2015 The Bitcoin Core developers

This comment has been minimized.

Copy link
@fanquake

fanquake Aug 8, 2016

Member

nit: Just 2016

@fanquake

View changes

depends/packages/rapidcheck.mk Outdated
$(package)_file_name:rapidcheck-1.0.tar.gz

$(package)_sha256_hash:c228dc21ec24618bfb6afa31d622d1f4ea71168f04ee499e1ffcfc63cd5833f4

This comment has been minimized.

Copy link
@fanquake

fanquake Aug 8, 2016

Member

Does this need $(package)_build_subdir=build ?

This comment has been minimized.

Copy link
@Christewart

Christewart Aug 21, 2016

Author Contributor

Not sure what you mean by this, do you mean we don't need lines 11 - 21?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

I think this is useful. I'm not very familiar with property based testing, but this seems to be something that would make sense for consensus and security critical projects.

Some thoughts:

  • the rapidcheck dependency should probably only be required if one passes --enable-tests.
  • I think you can remove the changes in src/test/script_tests.cpp and its header.

If others also agree to take this into master, we should probably have a first logical slice of property based tests (maybe serialization).

Needs rebase.

@jonasschnelli jonasschnelli added Feature Tests and removed Feature labels Aug 8, 2016

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 18, 2016

@theuni Can you help here to decide if using depends is fine or a subtree would be cleaner? (I prefer a subtree per my above comment, but I am not familiar with the build system. Input is appreciated)

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2016

I would probably need help with the build code to get it production ready. I plan to keep on adding properties to this pull request though when time permits. I'm going to try and get around implementing fanquake's comments and figure out how to do the --enable-test as per jonasschnelli's comment this weekend

@MarcoFalke

View changes

depends/packages/rapidcheck.mk Outdated

$(package)_version:1.0

$(package)_download_path:https://github.com/Christewart/rapidcheck/releases/download/1.0

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 19, 2016

Member

Again, I think a subtree is the better choice per my previous comment. Also, I don't think we can have someone verify this binary each time it is bumped, even if it was done deterministically.

This comment has been minimized.

Copy link
@theuni

theuni Aug 19, 2016

Member

Not sure what you mean about the deterministic bump. This is just a source tarball. Verifying would be no different than any of the other depends.

The general rule of thumb, though, is whether builders will be able to use it without any extra work. Since rapidcheck isn't present in any distros (as far as I can see), the answer to that is "no".

So, unless we're willing to require a depends build to run the new tests, a subtree would be easiest. However.

Tests are a bit of a special case. It may be reasonable to keep this in depends for now and let the c-i run it until there's sufficient coverage.

There's also the fact that it uses CMake, which we really don't want to introduce into our build (nothing against CMake, we just don't want to require every build tool under the sun).

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 19, 2016

Member

Ok, fine with me. Thanks for letting us know.

@Christewart Christewart force-pushed the Christewart:rapidcheck branch 4 times, most recently to 0a658bb Aug 21, 2016


$(package)_file_name=$(package)-$($(package)_version).tar.gz

$(package)_sha256_hash=c228dc21ec24618bfb6afa31d622d1f4ea71168f04ee499e1ffcfc63cd5833f4

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 24, 2016

Member

Can you try to adjust the folder name within the targz to have the version appened?

This may fix the travis issue.

This comment has been minimized.

Copy link
@Christewart

Christewart Aug 24, 2016

Author Contributor

I should note that I did NOT get any of this deterministic build working on my machine, I was trying to copy what I have seen on other other files to no avail -- I just ended up adding rapidcheck to my includes directory on my local machine. I'm fairly new to C++ & it's build systems so I was trying to just hack the pieces together. So I wouldn't be surprised if there are deeper issues than a naming issue although I will try and look at that later today. What exact name are you talking about? Are you talking about line 7? Line 5?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 24, 2016

Member

No worries, I don't understand cpp or the build system either. I can take a look later today... I think the issue is within the package you uploaded.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 25, 2016

Member

@Christewart Please try to fetch MarcoFalke@befb827 (Mf1608-rapidcheckRework) and push it here. This should fix your compile issues for now.

Please note that travis will likely reject this due to a bug in gcc4.8.x: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55914

define $(package)_preprocess_cmds
mkdir build
endef
$(package)_sha256_hash=78cdb8d0185b602e32e66f4e5d1a6ceec1f801dd9641b8a9456c386b1eaaf0e5

This comment has been minimized.

Copy link
@Christewart

Christewart Aug 26, 2016

Author Contributor

Why is this hash different than the one I provided?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 26, 2016

Member

The file size is different, so must should the hash.

The targz you provided also included the 1.8 MB .git subfolder, which is not required.

The hash I provided should be the hash of the tarball created by GitHub: https://github.com/emil-e/rapidcheck/archive/f5d3afa4f387ecf147faf98d96710a6edfa420f1.tar.gz

This comment has been minimized.

Copy link
@Christewart

Christewart Aug 26, 2016

Author Contributor

Ok that makes sense. Didn't realize you removed files. Do you have any idea why this is still failing?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 29, 2016

@Christewart Do you need help to address the feedback from @jonasschnelli?

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2016

@MarcoFalke Yeah I'm not really sure how to get that done

CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
ss << tx;
deserialize_type t;
CTransaction tx2(t,ss);

This comment has been minimized.

Copy link
@sipa

sipa Jan 8, 2017

Member

The intended idiom is just

CTransaction tx2(deserialize, ss);
@gmaxwell

This comment has been minimized.

Copy link
Member

commented Jan 8, 2017

I like this kind of testing-- testing invariants on random test cases--, and we use it extensively in libsecp256k1. It's one of the answers to my irritation with the way many of the unit tests in Bitcoin core have worked in the past: hard-coding the exact behavior. Meaning that if you change anything, the tests all fail and you're left wondering if you broke something important or if the test was just mandating the behavior you were fixing. In either case, updating the test is often more work than the change.

Taking a dependency for it seems unfortunate; but I'm not qualified to judge if it does snazzy things that make generating cases easy. I have had bad experience with test frameworks in the past which had very high overhead making test cases that should have taken seconds take minutes, resulting in less testing. I see in this one that it uses a excessively slow PRNG based on skein, which makes me a bit dubious-- but if thats the only problem it would be easy to fix if we forked it (by replacing it with xorshift128+ or likewise).

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2017

It should also be noted that they way the test are currently written they tie in the boost testing framework, if we add rapidcheck as a dependency and decide to remove boost, which #8670 suggests, we will have to rewrite these test cases to be independent of boost.

In terms of speed, you would know more than I would about choosing PRNG. Rapidcheck currently defaults to running 100 test cases against every property you have specified. You can configure this property to be higher or lower depending on the situation. I think it would make sense to configure this to lower value for local development, and a higher value for travis builds to try and exhaustively test our invariants. You can read more about configuration of rapidcheck here.

@jonasschnelli 's comment above was interesting I thought, I'm not super familiar with C++ build systems but only including the rapidcheck dependency if the flag --enable-test flag was given seems to makes sense.

I think the long term value add here is having a library of generators (for various protocol data structures) that we can use to test new protocol changes. I'm going to be adding some properties about creating tx types in the coming weeks. To get an idea of what I'm looking to add, you can look at some of the generators I have created in bitcoin-s.

@Christewart Christewart force-pushed the Christewart:rapidcheck branch 2 times, most recently Apr 7, 2017

@Christewart Christewart force-pushed the Christewart:rapidcheck branch Mar 24, 2018

Adding rapidcheck dependency, adding CKey properties
Adding rapidcheck dependency, adding CKey properties

Successfully compiling bitcoin with rapidcheck dependency

Adding new property file for CKey

Serialization symmetry for CKey -> CBitcoinSecret -> CKey

Adding generators for CPrivKey, CPubKey, and uint256

[depends] Rework rapidcheck.mk

Adding Bloom filter properties and bloom filter generator

Adding serialization symmetry for CBloomFilter

Adding block header serialization property

Fixing generator bug where I wasn't setting all fields on a BlockHeader

Creating transaction_gen.h, adding generator for COutPoint and serialization symmetry property for COutPoint

Adding script generator and script_propertes, first property is CScript serialization symmetry

Adding CTransaction generator, adding serialization symmetry property for CTransaction

Removing 'oneOrMoreInputs' and 'oneOrMoreOutputs' generators in favor of using rapidcheck's gen::nonEmpty function

Adding CTransactionRef and CBlock generators, adding property for CBlock serialization symmetry

Adding Generators outside of the rc name space, adding generator for LoadedBloomFilter

Adding merkleblock_gen.h, merkleblock serialization symmetry test

Adding merkle_block properties

Committing to try and debug mem leak

Removing comments, rebasing to master

Only adding rapidcheck dependency if tests are enabled

Refactoring inclues in generator files, reordering files in Makefile.test.include

Undoing change in merkleblock.cpp

Adding bloom_gen.cpp

Reworking BetweenZeroAndOne to use rc::gen::map instead of rc::gen::suchThat, removing some unused imports

Adding signedP2PKTx(), signedP2PKHTx(), signedMultisigTx(), and then a generator called signedP2SHTx() that creates a p2sh output and spends it

Adding witness SPKs and witness spending transactions

Making sure helpers are functions and not constant values

fixing nits/ugliness

Adding block_gen.cpp

Modifying code to be closer to the style guide rules, use const/references, fixing more nits

autotools/depends fixes for rapidcheck

Fixes non-depends bitcoin build when rapidcheck is not installed on system,
by adding --with-rapidcheck config option.

Fixes depends build by adding fPIC option and installation steps so the build
is not empty. Allows property-based tests to be run with:

    make -C depends
    ./configure --prefix=$PWD/depends/x86_64-pc-linux-gnu
    make check

depends: Bump rapidcheck to version 10fc0cb

depends: Disable RAPIDCHECK by default

run clang-format

remove trailing white space

remove native ccache in packages.mk
@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2018

I believe the travis failure here is unrelated?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

The last travis run for this pull request was 113 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2018

Needs rebase
@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@Christewart This is excellent work! Do you have time to rebase this and take it to completion, or do you prefer if someone else takes over this work? It is too good to be left unfinished :-)

@Christewart

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2018

Hi @practicalswift it seems like the consensus is moving to #12775 which is a subset of the PR. It includes just the build stuff and a subset of the tests in this PR.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@Christewart Oh, sorry! That one is rebased and nice! Thanks!

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Sep 8, 2018

Merge bitcoin#12775: Integration of property based testing into Bitco…
…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

@MarcoFalke MarcoFalke removed the Needs rebase label Sep 9, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2018

Needs rebase
#include <rapidcheck/Gen.h>


typedef std::tuple<int32_t, uint256, uint256, uint32_t, uint32_t, uint32_t> BlockHeaderTup;

This comment has been minimized.

Copy link
@Empact

Empact Oct 8, 2018

Member

nit: using?

});
}

rc::Gen<unsigned int> Between1And100()

This comment has been minimized.

Copy link
@Empact

Empact Oct 9, 2018

Member

nit: looks like this method is defined with the same name here and in merkleblock_gen.h, how about:

  • applying static more liberally?
  • renaming one or the other?
@Empact

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Moving my review to #14430, which along with #12775 was extracted from this. @Christewart should this be closed?

@fanquake

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

This was superseded by #12775 and #14430.

@fanquake fanquake closed this Oct 10, 2018

@fanquake fanquake removed the Needs rebase label Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.