Switch testing framework from MAIN to new UNITTEST network #4845

SergioDemianLerner · 2014-09-04T19:46:54Z

This is our third attempt to improve the testing framework taking into account all previous comments and suggestions. The two previous attempts are: pull requests #4688 and #4732.

Summary: For several reasons the current test framework does not allow to easily incorporate new unit tests that append specially crafted blocks to the blockchain using ProcessBlock().
After we proposed a solution in #4688 @gmaxwell and @laanwj suggested not implementing the changes because many files not directly related to the testing framework were modified by the patch.
In the pull request #4732 we implement @gmaxwell suggestion that the test framework should run under the RegTest network parameters and not under the main network parameters. It was discovered that 10 unit cases are designed for the MAIN net, RegTest inherits from TestNet, which has many differences, so switching is not simple nor desired. @mikehearn and @laanwj proposed using a special new network called UNITTEST to be as similar to the real net as possible. We implemented this final suggestion.

In this pull request we implement UNITTEST. We reintroduced the criticized possibility to disable PoW check, but we did it under Params().SkipProofOfWorkCheck(). This method is a const virtual function that returns false for MAIN net, so there is no possibility that it is switched to true, under normal circumstances. This change does not extend the attack surface in any way.

The child network UNITTEST overrides SkipProofOfWorkCheck() to provide the skipping functionality.
To allow test cases to modify network parameters, we've added the ModifiableParams() global variable and associated abstract interface, with setters. These setters are only available if the current network is UNITTEST. Again, there is no way to modify the network parameters if the current network is not UNITTEST.

As in the previous pull requests, we've:

a. fixed the bug in miner_tests.cpp which leaves in the memory pool invalid transactions (mempool.clear() missing).
b. Added 7 unit tests:

ToCheckBlockUpgradeMajority (untested before)
EnforceBlockUpgradeMajority (untested before)
RejectBlockOutdatedMajority (untested before)
"bad-cb-height"
"bad-version"
"time-too-old"
"bad-txns-nonfinal"

Sergio Demian Lerner & Timo Hanke

UNITTEST inherites from MAIN but allows synamically changing its parameters using the ModifiableParams() interface

Diapolo · 2014-09-05T08:06:29Z

src/main.cpp

@@ -2330,6 +2330,7 @@ bool AcceptBlockHeader(CBlockHeader& block, CValidationState& state, CBlockIndex
        nHeight = pindexPrev->nHeight+1;

        // Check proof of work
+        if (!Params().SkipProofOfWorkCheck())


This looks weird, is it missing an indentation below or some brackets?

Or just fold it into the expression with &&

It's missing indentation below.
But this is on purpose: I want critical consensus code not even look as modified so everyone can evaluate security at eyesight.

mikehearn · 2014-09-05T09:57:53Z

Looking good! Most of my comments are minor style points. 👍

Also new test case testing the PoW skip in UNITTEST.

BitcoinPullTester · 2014-09-09T16:40:52Z

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4845_eda4aa19d3cafc1644f5c28c687741a6c255b9a6/ for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

mikehearn · 2014-09-09T18:31:32Z

Great, thanks! I didn't try running the tests myself but the code looks good to me.

SergioDemianLerner · 2014-09-10T21:54:42Z

I would like to know if there is a plan to add this patch to master or a reason not to. I have another patch waiting with more test cases that requires this patch to work! Thanks.

sipa · 2014-09-10T23:20:56Z

I haven't reviewed all code, but the changes to the existing core code and tests do seem safe to me.

laanwj · 2014-09-29T11:05:17Z

Had to be rebased for the clang code formatting change, as well as some other (base)chainparams changes.
Rebased version here: https://github.com/laanwj/bitcoin/tree/2014_09_unittest_master

laanwj · 2014-09-29T11:08:11Z

I'm not too happy about a virtual being introduced into CChainParams. Can we avoid this?

virtual bool SkipProofOfWorkCheck() const { return false; }

Edit: yes we can, see 4705902

laanwj · 2014-09-29T11:20:06Z

ACK - will merge after you've sanity-checked my rebased version.

4705902 Avoid introducing a virtual into CChainParams (Wladimir J. van der Laan) 5e2e7fc Suggested corrections on comments, variable names. Also new test case testing the PoW skip in UNITTEST. (SergioDemianLerner) a25fd6b Switch testing framework from MAIN to new UNITTEST network (SergioDemianLerner)

laanwj · 2014-10-02T08:33:32Z

Oops - this was marked by Travis as ok, but is failing the windows tests now after I tried to merge.

See

Lots of:

test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed
test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed
test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed
test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed

See #5030.

It breaks the new mingw tests! See - https://travis-ci.org/bitcoin/bitcoin/jobs/36845581 - https://travis-ci.org/bitcoin/bitcoin/jobs/36845582 This reverts commit 4705902, 5e2e7fc, a25fd6b.

SergioDemianLerner · 2014-10-02T14:06:14Z

@laanwj I asked TheBlueMatt before about this problem. I could not replicate the problem, but It think that in some platforms the order in which the test cases are executed is different. Ohh, you already discovered this in #5030

laanwj · 2014-10-08T07:11:23Z

Closing this. The modifiable parameters part has been merged (see #5030). The blocksv2 test case that relies on being executed in a specific order w/ miner_tests has not, as that's not acceptable (as it makes the test framework non-deterministic, and makes it impossible to run testcases separately). Please find a different solution for that.

Switch testing framework from MAIN to new UNITTEST network

357c35c

UNITTEST inherites from MAIN but allows synamically changing its parameters using the ModifiableParams() interface

Diapolo reviewed Sep 5, 2014
View reviewed changes

Suggested corrections on comments, variable names.

eda4aa1

Also new test case testing the PoW skip in UNITTEST.

laanwj mentioned this pull request Oct 2, 2014

Switch testing framework from MAIN to new UNITTEST network (rebase) #5030

Merged

laanwj closed this Oct 8, 2014

laanwj mentioned this pull request Sep 28, 2016

chainparams: Added parametric halving interval for regtest-only mode #8623

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch testing framework from MAIN to new UNITTEST network #4845

Switch testing framework from MAIN to new UNITTEST network #4845

SergioDemianLerner commented Sep 4, 2014

Diapolo Sep 5, 2014

mikehearn Sep 5, 2014

SergioDemianLerner Sep 5, 2014

mikehearn commented Sep 5, 2014

BitcoinPullTester commented Sep 9, 2014

mikehearn commented Sep 9, 2014

SergioDemianLerner commented Sep 10, 2014

sipa commented Sep 10, 2014

laanwj commented Sep 29, 2014

laanwj commented Sep 29, 2014

laanwj commented Sep 29, 2014

laanwj commented Oct 2, 2014

SergioDemianLerner commented Oct 2, 2014

laanwj commented Oct 8, 2014

Switch testing framework from MAIN to new UNITTEST network #4845

Switch testing framework from MAIN to new UNITTEST network #4845

Conversation

SergioDemianLerner commented Sep 4, 2014

Diapolo Sep 5, 2014

Choose a reason for hiding this comment

mikehearn Sep 5, 2014

Choose a reason for hiding this comment

SergioDemianLerner Sep 5, 2014

Choose a reason for hiding this comment

mikehearn commented Sep 5, 2014

BitcoinPullTester commented Sep 9, 2014

mikehearn commented Sep 9, 2014

SergioDemianLerner commented Sep 10, 2014

sipa commented Sep 10, 2014

laanwj commented Sep 29, 2014

laanwj commented Sep 29, 2014

laanwj commented Sep 29, 2014

laanwj commented Oct 2, 2014

SergioDemianLerner commented Oct 2, 2014

laanwj commented Oct 8, 2014