Switch testing framework from MAIN to new UNITTEST network

SergioDemianLerner commented at 7:46 pm on September 4, 2014: contributor

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

Switch testing framework from MAIN to new UNITTEST network

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

357c35cb47

in src/main.cpp: in 357c35cb47 outdated

2329@@ -2330,6 +2330,7 @@ bool AcceptBlockHeader(CBlockHeader& block, CValidationState& state, CBlockIndex
2330         nHeight = pindexPrev->nHeight+1;
2331 
2332         // Check proof of work
2333+        if (!Params().SkipProofOfWorkCheck())

Diapolo commented at 8:06 am on September 5, 2014:

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

mikehearn commented at 9:48 am on September 5, 2014:

Or just fold it into the expression with &&

SergioDemianLerner commented at 12:50 pm on September 5, 2014:

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.

in src/chainparams.cpp: in 357c35cb47 outdated

213@@ -214,8 +214,50 @@ class CRegTestParams : public CTestNetParams {
214 };
215 static CRegTestParams regTestParams;
216 
217+//
218+// Regression test

mikehearn commented at 9:47 am on September 5, 2014:

Would be good to s/Reg/Unit/ here to keep things distinct. Regtest is for programs that drive the app externally over the wire, unit tests is for, well, unit tests.

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

28+        // Add the coinbase
29+        CMutableTransaction txCoinbase(pblock->vtx[0]);
30+        
31+        if (addHeight) 
32+            txCoinbase.vin[0].scriptSig = (CScript() << (chainActive.Height()+1+difValue) << 0);
33+            else

mikehearn commented at 9:50 am on September 5, 2014:

Indentation. Also watch for whitespace around function parameter lists and so on. You might want to call the variable “diffValue” as “dif” is not a common way to abbreviate a word like difficulty.

SergioDemianLerner commented at 12:53 pm on September 5, 2014:

difValue is a difference from the current block height, not for difficulty. I can rename it to heightDifference.

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

40+
41+void CheckSubsidyHalving(CBlockTemplate * &pblocktemplate, CBlock * &pblock)
42+{
43+    if ((chainActive.Height()+1) % Params().SubsidyHalvingInterval() == 0)
44+        {
45+            // The RegTest network has a low subsidy halving interval (150) so

mikehearn commented at 9:51 am on September 5, 2014:

s/RegTest/Unittest/

Indentation is wrong.

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

66+    // If miner_tests.cpp is run before, the chain will be 100 blocks long, and all of them will be v1
67+
68+    
69+    LogPrintf("Blockv2test testcase starts\n"); 
70+        
71+    CBlockTemplate *pblocktemplate;

mikehearn commented at 9:52 am on September 5, 2014:

I’d feel better when things like this are scoped_ptr<>. Raw pointers in C++ are too easy to mess up.

sipa commented at 11:19 pm on September 10, 2014:

+1

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

88+    CValidationState state1;
89+    PreviousHeight = chainActive.Height();
90+    BOOST_CHECK(ProcessBlock(state1, NULL, pblock));
91+    BOOST_CHECK(state1.IsValid());
92+    BOOST_CHECK((PreviousHeight+1) == chainActive.Height()); // to differentiate from orphan blocks, which also get accepted in ProcessBlock()	
93+    pblock->hashPrevBlock = pblock->GetHash(); // update parent

mikehearn commented at 9:53 am on September 5, 2014:

This line confuses me. You’re setting hashPrevBlock to the hash of the containing block, no? Or that’s what it looks like. I don’t see where the blocks parent is being modified.

sipa commented at 9:59 am on September 5, 2014:

This is weird and surprising, but it’s just copied from miner_tests.cpp.

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

109+    // "bad-txns-BIP30" because the coinbase tx has the same hash as the previous.
110+    // Unluckily, even if ConnectBlock returns a "bad-txns-BIP30", ActivateBestChainStep clears
111+    // the state, so we get true here and the "bad-txns-BIP30" reason is lost.
112+    // We verify instead that the chain height has not been incremented.
113+    
114+    CValidationState state7;

mikehearn commented at 9:54 am on September 5, 2014:

It might make sense to refactor this “add a block, did it work” logic into a function.

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

106+    LogPrintf("Blockv2test BIP30 repetition begin\n");  
107+
108+    // First, if we try to add a block v2 with the same coinbase tx, we should get
109+    // "bad-txns-BIP30" because the coinbase tx has the same hash as the previous.
110+    // Unluckily, even if ConnectBlock returns a "bad-txns-BIP30", ActivateBestChainStep clears
111+    // the state, so we get true here and the "bad-txns-BIP30" reason is lost.

mikehearn commented at 9:55 am on September 5, 2014:

This “reason got lost” thing sounds like a bug. Is it? No worries about fixing it in this patch if so, I’m just interested.

sipa commented at 10:02 am on September 5, 2014:

No, it’s not a bug. Receiving a single block can cause zero or multiple blocks to be connected, and ActivateBestChain’s responsibility is just switching the best block whatsoever.

Feedback about failures causes a reject message to be sent to the peer from which we received the actual block (not necessarily the same as from whom we got the block that caused the reorg), for which we remember the peerid. It would be useful to generalize this mechanism to have generic callbacks available about block validation (for example for BIP23 rejection reasons), which could also be used here.

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

172+        pblock->hashPrevBlock = pblock->GetHash(); // update parent
173+    }
174+
175+    
176+    LogPrintf("Blockv2test block v1 rejected\n");   
177+    // Now we add 200 additional blocks, until we get 950 (the threshold were v1 blocks are not accepted anymore)

mikehearn commented at 9:56 am on September 5, 2014:

s/were/where/

mikehearn commented at 9:56 am on September 5, 2014:

Also looks like this comment should be attached to the block above, I think. You’re not adding 200 blocks at this point, that was just done.

in src/test/blockv2_tests.cpp: in 357c35cb47 outdated

185+    BOOST_CHECK(state2.GetRejectReason()=="bad-version");
186+    // Do not update parent since block has failed
187+    
188+    
189+    
190+    // Some other missing tests, added here as bonus...

mikehearn commented at 9:57 am on September 5, 2014:

Could these be in separate unit tests?

SergioDemianLerner commented at 1:02 pm on September 5, 2014:

Yes, but since they create v2 blocks, they can only be run after the previous v2 majority tests. So there is a hidden dependence.

mikehearn commented at 9:57 am on September 5, 2014: contributor

Looking good! Most of my comments are minor style points. :+1:

Suggested corrections on comments, variable names.

Also new test case testing the PoW skip in UNITTEST.

eda4aa19d3

BitcoinPullTester commented at 4:40 pm on September 9, 2014: none

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 commented at 6:31 pm on September 9, 2014: contributor

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

SergioDemianLerner commented at 9:54 pm on September 10, 2014: contributor

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 commented at 11:20 pm on September 10, 2014: member

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

laanwj commented at 11:05 am on September 29, 2014: member

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 commented at 11:08 am on September 29, 2014: member

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

0virtual bool SkipProofOfWorkCheck() const { return false; }

Edit: yes we can, see 4705902

laanwj commented at 11:20 am on September 29, 2014: member

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

laanwj referenced this in commit ad51e14583 on Oct 2, 2014

laanwj commented at 8:33 am on October 2, 2014: member

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

See

Lots of:

0test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
1test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed
2test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
3test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed
4test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
5test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed
6test/miner_tests.cpp(82): error in "CreateNewBlock_validity": check ProcessBlock(state, __null, pblock) failed
7test/miner_tests.cpp(83): error in "CreateNewBlock_validity": check state.IsValid() failed

See #5030.

laanwj referenced this in commit 8d132431b4 on Oct 2, 2014

SergioDemianLerner commented at 2:06 pm on October 2, 2014: contributor

@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 commented at 7:11 am on October 8, 2014: member

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.

laanwj closed this on Oct 8, 2014

MarcoFalke locked this on Sep 8, 2021

Switch testing framework from MAIN to new UNITTEST network #4845