Add more property based tests for basic bitcoin data structures #14430
Conversation
|
Concept ACK Very nice! Thanks for working on this |
|
Ref #12775 which this builds on. |
| uint32_t nTime; | ||
| uint32_t nBits; | ||
| uint32_t nNonce; | ||
| std::tie(nVersion, hashPrevBlock, hashMerkleRoot, nTime, nBits, nNonce) = primitives; |
There was a problem hiding this comment.
Seems you could directly tie into the header fields, to make this more succinct.
There was a problem hiding this comment.
Not sure what you mean about this
|
How about responding to @MarcoFalke's suggestions? |
| }); | ||
| } | ||
|
|
||
| rc::Gen<unsigned int> Between1And100() |
There was a problem hiding this comment.
nit: looks like this method is defined with the same name here and in merkleblock_gen.h, how about:
- applying
staticmore liberally? - renaming one or the other?
There was a problem hiding this comment.
So basically mark as static in merkleblock_gen.h and then include that header file and use it here?
There was a problem hiding this comment.
Well they have different implementations for different return results. Calling this 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.
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 Lines 3112 to 3118 in 4de0b5f |
| } | ||
|
|
||
| /** Loads an arbitrary bloom filter with the given hashes */ | ||
| rc::Gen<std::pair<CBloomFilter, std::vector<uint256>>> LoadBloomFilter(const std::vector<uint256>& hashes) |
There was a problem hiding this comment.
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 :-)
|
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. 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. |
|
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 since we needs extra functionality located in this package https://github.com/bitcoin-core/rapidcheck/tree/master/extras specifically the It would be much appreciated if someone who is more adept at |
|
@Christewart have you seen this? https://github.com/emil-e/rapidcheck/blob/master/doc/boost_test.md |
|
@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 since we integrate with 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 emil-e/rapidcheck#222 (comment) We will need to update the rapidcheck dependency to at least emil-e/rapidcheck@5f0eb30 |
|
@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): The entire build output is available here. |
ac67582 depends: latest rapidcheck, use INSTALL_ALL_EXTRAS (fanquake) Pull request description: This updates RapidCheck to the latest version available from https://github.com/emil-e/rapidcheck. RapidCheck now uses the new `RC_INSTALL_ALL_EXTRAS` option, to install the extra `boost_test` packages, which should unblock progress in #14430. ACKs for commit ac6758: MarcoFalke: utACK ac67582 Tree-SHA512: a4a4ef0ec09cf61cdc0de241703f5f8e98f6fa92f4024a0fbbf4d4ef91d9d3bc8d662c55d896aced8de68aa9429728b2bc5001c91c6f92d63d60c47f5adf41a0
|
@Christewart Could you rebase this, now that #14853 has been merged? |
Christewart
force-pushed
the
add_tests_from_8469
branch
from
db8a8c0
to
48740ba
Compare
|
Rebased this branch, but i am getting a new error when running these tests locally. This doesn't seem related to my PR AFAICT |
|
@Christewart Yes, those annoying errors are unrelated: see discussion in #15352 :-) |
There was a problem hiding this comment.
@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.
|
@fanquake I wrote @Christewart about this PR 3 days ago and he might have some time this week. |
Christewart
force-pushed
the
add_tests_from_8469
branch
from
48740ba
to
2c2e3c9
Compare
|
So i rebased onto master, and fixed various things that needed to be fixed with API changes in master.
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 don't think rapidcheck can be installed with brew (admittedly, I don't have a mac). I think you need to do rapidcheck $ cmake . && make install
-- Configuring done
-- Generating done
-- Build files have been written to: /home/chris/dev/rapidcheck
[100%] Built target rapidcheck
Install the project...
-- Install configuration: ""
-- Up-to-date: /usr/local/lib/librapidcheck.a
-- Up-to-date: /usr/local/include
-- Up-to-date: /usr/local/include/rapidcheck.h
-- Up-to-date: /usr/local/include/rapidcheck
....
-- Up-to-date: /usr/local/include
-- Up-to-date: /usr/local/include/rapidcheck
-- Up-to-date: /usr/local/include/rapidcheck/boost_test.h
|
There was a problem hiding this comment.
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 emil-e/rapidcheck@d9482c6.
CXX test/gen/test_bitcoin-block_gen.o
CXX test/gen/test_bitcoin-bloom_gen.o
CXX test/gen/test_bitcoin-crypto_gen.o
CXX test/gen/test_bitcoin-script_gen.o
CXX test/gen/test_bitcoin-transaction_gen.o
[...]
CXX test/test_bitcoin-block_properties.o
CXX test/test_bitcoin-bloom_properties.o
CXX test/test_bitcoin-key_properties.o
CXX test/test_bitcoin-merkleblock_properties.o
CXX test/test_bitcoin-script_properties.o
CXX test/test_bitcoin-transaction_properties.o
[...]
Running tests: block_properties from test/block_properties.cpp
Running tests: bloom_properties from test/bloom_properties.cpp
Running tests: key_properties from test/key_properties.cpp
Running tests: merkleblock_properties from test/merkleblock_properties.cpp
Running tests: script_properties from test/script_properties.cpp
Running 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 emil-e/rapidcheck#232 filed by @fanquake hasn't been addressed yet.
| @@ -153,11 +153,25 @@ BITCOIN_TESTS =\ | |||
|
|
|||
| if ENABLE_PROPERTY_TESTS | |||
| BITCOIN_TESTS += \ | |||
| test/key_properties.cpp | |||
| test/block_properties.cpp \ | |||
There was a problem hiding this comment.
nit: could update "2013-2016" in this file's copyright header
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
Christewart
force-pushed
the
add_tests_from_8469
branch
from
330465c
to
92c08f5
Compare
|
@jonatack I think everything is good to go now? |
|
Closing this in light of #18514. |
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.