Switch of the test framework from MAIN to REGTEST

[SergioDemianLerner]

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().
In the pull request #4688 we described the reasons and proposed a solution.
While we think that is the optimum solution, @gmaxwell and @laanwj suggested not implementing the changes because many files not directly related to the testing framework were modified by the patch.
In this pull request we implement @gmaxwell suggestion that the test framework should run under the RegTest network parameters and not under the main network parameters. Allthough this seems to be a small change, it was not. First, the RegTest block solving probability was 1/2 which still requires "mining" during the test case validation in order to dynamically create a block, which adds unnecessary complications to simple testing code.
To overcome this problem, the RegTest difficulty was changed from 0x207fffff to 0x2100FFFF (in compact notation), which gives an approximate 1/2^16 probability not to solve a block. Happily there was no lucky test case.
Because the maximum possible target (0xff ... 0xff) cannot be multiplied by 4 without overflow, ComputeMinWork() had to be modified to always return ProofOfWorkLimit() for the RegTest.

Also we found that several test cases make explicit use of properties of the MainNet. Such test cases are:

alert_tests.cpp
rpc_wallet_tests.cpp
main_tests.cpp
key_tests.cpp
DoS_tests.cpp
Checkpoints_tests.cpp
base58_tests.cpp
bloom_tests.cpp
rpc_tests.cpp
miner_tests.cpp

All these test cases were added a temporary switch from the RegTest network to the Main network in order to leave them working. Re-writting all of them for the RegTest probably requires more than 40 hours of work.

The alert_tests.cpp test case is special, because it requires the alert private key to generate inputs to the test cases. We suggest to switch this test case to the RegNet and replace the current RegTest alert private key with a published key-pair, so everyone can generate more test cases and there is no opaque data.

As the 4688 pull request, 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"

NOTE: In the file rpc_wallet_tests.cpp, the TABs were removed and replaced with whitespace indentation. This is the standard in the rest of the code. A whitespace-ignoring compare will show no important differences.

Sergio Demian Lerner & Timo Hanke

[BitcoinPullTester]

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

This could happen for one of several reasons:

1. 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)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. 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.

[SergioDemianLerner]

test/miner_tests.cpp(47) failed in BitcoinPullTester but does not fail in my own fork.
Could it be that test cases are processed in a different order in two different test_bitcoin compilations?
Or maybe other commits broke it..

I would be good if the fPrintToConsole were set to true during the test case execution, so the pull tester test.log had more information regarding the problem.

[mikehearn]

Is it really needed to change the definition of regtest mode? Bear in mind bitcoinj has the same constants in them. Mining two blocks is not a big deal, at least it was not for us (the tests are still plenty fast).

[laanwj]

ACK on the overall idea.

I'd also prefer not change the definition of regtest network. At the very least the merits and disadvantages have to be considered separately from the rest of this pull. Regtest as it is now has become a de-facto standard, so some other tools and libraries would have to be changed as well.

[laanwj]

We just got rid of almost all the Params().NetworkID()==CBaseChainParams::REGTEST. The idea is to define a bit on the Params() object for each point of difference between chains, not compare the network identifier.

[SergioDemianLerner]

Yes, I was expecting you tell me this. I will remove this line later. I just let it there to make clear that there is a problem with the multiplication code that must be addressed if difficulty is going to be zero.

[SergioDemianLerner]

@mikehearn With Timo Hanke we discussed this and we had the same impression. We suggest creating another regression testing network parameters, called MAIN_REGTEST that inherits from MAIN and it just changes the block difficulty.
Then we won't need any modification on the previous test-cases, because all of them will use the same MAIN parameters as the MAIN_REGTEST.

To summarize this pull request and #4688 we have 4 possibilities:

1. Let the test framework run in MAIN and add the suppression of block work global variable (this is Improvement to the Test Framework in the processing of test blocks #4688). Problem: some do not like adding this variable.
2. Switch the framework to REGTEST. Problem: some test cases use MAIN parameters and we don't want to break the REGTEST codes of other Bitcoin implementations.
3. Keep using MAIN for the test framework. Problem: Is ugly. Needs mining. Makes debugging harder. GMaxwell thinks MAIN was not intended for the testing framework, but REGTEST was.
4. Switch the framework to a new MAIN_REGTEST network inheriting from MAIN, but sets difficulty to zero. Very simple. Add 10 lines of code. Everything keeps working as is. Does not break anything.

I still think that 1 was the best choice.
But from peoples comments I suppose number 4 would be an consented choice.

[mikehearn]

Yes that's sort of what we do in bitcoinj too, except we call it the UNITTEST network instead of MAIN_REGTEST which is perhaps a confusing name. But having network parameters specific to unit testing is indeed quite useful.

[laanwj]

I'm not sure it is necessary to use one chain for all the testcases. Some testcases could run as MAIN, to verify parameters from main. Some, that need block generation, could run as REGTEST. Another test could use yet another parameter set.

Besides that (4) sounds pretty elegant. It keeps all the changes restricted to CChainParams. At some point in the future that could be extended and testcases could provide their own pointer to custom chain parameters to use, if they require a special custom setup.

[SergioDemianLerner]

@mikehearn Ok, let's call it UNITTEST.

@laanwj I agree. But one must take into account that switching network parameters is only possible for test cases that do not call ProcessBlock(). All test cases that call ProcessBlock() must probably share the same parameters to prevent incompatibilities (which is not that bad) . If we'd really like complete free control of the block-chain for each unit test, then we would need to incorporate the block-chain destroy/creation code present in #4688.

[SergioDemianLerner]

@laanwj Do we agree that if I create a new network parameters UNITTEST we'll merge this patch?

[jtimon]

Maybe we should just regtest's behavior to be more like mainnet, should be easy to do in chainparams.
What are the use cases that would be broken if we do that?
Maybe we can even get rid of some of the flags in chainparams that are specific to regtest.
I wouldn't oppose to the creation of a new mode that's a hybrid between mainnet and regtest though, and UNITEST sounds good to me. Again, this should much simpler to do now that there's no Params().NetworkID() Params().isMainNet() and Params().isTestNet() all around.
So anyway, NACK on all new uses of Params().NetworkID() maybe besides checkpoint_test, I just cleaned that up not too long ago. The remaining uses of NetworkID() (isMainet and isTestnet has been removed) are:

1. A switch in checkpoints.cpp to select the data.
2. A couple of Params().NetworkID() == CBaseChainParams::TESTNET for "testnet" fields that should be deprecated as soon as we break rpc backwards compatibility in rpcmisc and rpcmining
3. A Params().NetworkID() != CBaseChainParams::MAIN check in qt/bitcoin.cpp to select orange or green.

If it's possible we should get rid of those, not create more.

[jtimon]

In other words, you should be able to write this PR on top of #4802.
EDIT: sorry, I missed that you already said you were going to remove them later...

[laanwj]

Needs rebase.