test: remove rapidcheck integration and tests

fanquake commented at 11:29 am on April 3, 2020: member

Whilst the property tests are interesting, ultimately rapidcheck integration in this repository has not gained much traction. We have a limited number of tests, and they are rarely (if ever) run. Have discussed this with Chris Stewart.

fanquake added the label Build system on Apr 3, 2020

fanquake added the label Tests on Apr 3, 2020

fanquake requested review from Christewart on Apr 3, 2020

Christewart commented at 11:30 am on April 3, 2020: member

ACK, from talking with @MarcoFalke it seems that his/bluematt’s fuzzing work has superseded this

jonatack commented at 11:32 am on April 3, 2020: member

I may be a minority, but I run them on every build multiple times a day. Still meaning to review #14430.

MarcoFalke commented at 11:40 am on April 3, 2020: member

ACK. unit and fuzz tests are sufficient.

The tests here are better written as unit tests, in which case they also run on all platforms and all dev machines.

MarcoFalke commented at 11:54 am on April 3, 2020: member

I may be a minority, but I run them on every build multiple times a day. Still meaning to review #14430.

I looked over them and they are better suited either as unit or fuzz tests. If you plan to go over this pull, you could move and transpile them to the appropriate unit test of fuzz test file.

It looks like even travis isn’t running the property based test. Unit and fuzz tests are run with sanitizers or valgrind turned on. So instead of wasting effort on an experimental and mostly not even compiled testing solution, we should streamline our effort one two that are already widely supported and run.

jonatack commented at 12:01 pm on April 3, 2020: member

My impression was that PBT could find different edge cases and stress the code differently from unit tests (which only cover the cases the programmer thought of) and fuzzing, which looks for crashes from inputs. I understand not wasting time and don’t want to obstruct consensus, but it might be worthwhile to leave them for any additional coverage they may provide in the future.

MarcoFalke commented at 12:23 pm on April 3, 2020: member

I just can’t imagine that property based tests could ever come up with more coverage than our unit or fuzz tests can povide.

Let’s look at this example:

0RC_ASSERT(!(key1 == key2));`

where the key is generated by this:

0            CKey key;
1            key.MakeNewKey(true);

The unit test would look like:

0CKey key1, key2;
1key1.MakeNewKey(true), key2.MakeNewKey(true);
2BOOST_CHECK(!(key1 == key2));

MarcoFalke commented at 12:34 pm on April 3, 2020: member

I mean if someone comes along and has a great test case that can only be achieved with property based tests, sure I am happy to keep them along. But this hasn’t happened in the last three years, nor will it happen in the next three years. And if it did, we can always reconsider and call git revert bafc471a994948e1f4c64bbea101da200347b0eb.

test: remove rapidcheck integration and tests 9e071b0089

fanquake force-pushed on Apr 3, 2020

practicalswift commented at 3:42 pm on April 3, 2020: contributor

Concept ACK: keeping an inactive rapidcheck integration around does not feel meaningful

If someone in the future comes up with an example where a rapidcheck test outperforms standard coverage-based fuzzing we can always revert this change.

ACK, from talking with @MarcoFalke it seems that his/bluematt’s fuzzing work has superseded this

Is @TheBlueMatt fuzzing Bitcoin Core without sharing his fuzzing harnesses with us? :D

DrahtBot commented at 7:45 pm on April 3, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18077 (net: Add NAT-PMP port forwarding support by hebasto)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16367 (Multiprocess build support by ryanofsky)
#15382 (util: add runCommandParseJSON by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift commented at 1:51 pm on April 4, 2020: contributor

ACK 9e071b00898aedd9632f105a22d976dc6dbc84b1

fanquake merged this on Apr 6, 2020

fanquake closed this on Apr 6, 2020

fanquake deleted the branch on Apr 6, 2020

jb55 commented at 2:16 am on April 6, 2020: member

Property based testing only gets interesting over fuzzing when you get in the realm of state-machine testing. ie. generating a random sequence of actions/api calls and checking them against a model of what you expect the state to be (see https://www.youtube.com/watch?v=zi0rHwfiX1Q).

rapidcheck didn’t seem like the greatest library either.

ACK

MarcoFalke commented at 1:57 pm on April 6, 2020: member

Property based testing only gets interesting over fuzzing when you get in the realm of state-machine testing. ie. generating a random sequence of actions/api calls and checking them against a model of what you expect the state to be

This is one of the strengths of fuzzing. See for example the fuzzer to find the money printing consensus failure #17860 .

The utxo set is the state and the maximum money supply is the condition you check. The actions are

0enum class Action : uint8_t {
1    CREATE_INPUT, //!< Append an input-output pair to the last tx in the current block
2    CREATE_TX,    //!< Append a tx to the list of txs in the current block
3    CREATE_BLOCK, //!< Append the current block to the active chain
4};

MarcoFalke commented at 1:58 pm on April 6, 2020: member

TLDR property based testing is fuzz testing

jb55 commented at 4:32 pm on April 6, 2020: member

Yes from the code you linked it looks like we’re already doing a form of property based testing using FuzzedDataProvider. Good stuff!

jasonbcox referenced this in commit 40964aa339 on Aug 20, 2020

deadalnix referenced this in commit 4c32b05452 on Aug 21, 2020

DrahtBot locked this on Feb 15, 2022

test: remove rapidcheck integration and tests #18514

Conflicts