Suggested corrections on comments, variable names.

Also new test case testing the PoW skip in UNITTEST.

3 files changed, 91 insertions, 48 deletions

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index d924a6a2b0..8a00da0bb5 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -215,7 +215,7 @@ public:
 static CRegTestParams regTestParams;
 
 //
-// Regression test
+// Unit test
 //
 class CUnitTestParams : public CMainParams, public CModifiableParams {
 public:
@@ -242,9 +242,9 @@ public:
     virtual void setEnforceBlockUpgradeMajority(int anEnforceBlockUpgradeMajority)  { nEnforceBlockUpgradeMajority=anEnforceBlockUpgradeMajority; }
     virtual void setRejectBlockOutdatedMajority(int anRejectBlockOutdatedMajority)  { nRejectBlockOutdatedMajority=anRejectBlockOutdatedMajority; }
     virtual void setToCheckBlockUpgradeMajority(int anToCheckBlockUpgradeMajority)  { nToCheckBlockUpgradeMajority=anToCheckBlockUpgradeMajority; }
-    virtual void setDefaultCheckMemPool(bool aDefaultCheckMemPool)  { fDefaultCheckMemPool=aDefaultCheckMemPool; }
-    virtual void setAllowMinDifficultyBlocks(bool aAllowMinDifficultyBlocks) {  fAllowMinDifficultyBlocks=aAllowMinDifficultyBlocks; }
-    virtual void setSkipProofOfWorkCheck(bool aSkipProofOfWorkCheck) { fSkipProofOfWorkCheck = aSkipProofOfWorkCheck; }
+    virtual void setDefaultCheckMemPool(bool afDefaultCheckMemPool)  { fDefaultCheckMemPool=afDefaultCheckMemPool; }
+    virtual void setAllowMinDifficultyBlocks(bool afAllowMinDifficultyBlocks) {  fAllowMinDifficultyBlocks=afAllowMinDifficultyBlocks; }
+    virtual void setSkipProofOfWorkCheck(bool afSkipProofOfWorkCheck) { fSkipProofOfWorkCheck = afSkipProofOfWorkCheck; }
 };
 static CUnitTestParams unitTestParams;
 
diff --git a/src/main.cpp b/src/main.cpp
index 4bd8fd6e36..7e60fd4ff9 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -2304,8 +2304,8 @@ bool AcceptBlockHeader(CBlockHeader& block, CValidationState& state, CBlockIndex
         nHeight = pindexPrev->nHeight+1;
 
         // Check proof of work
-        if (!Params().SkipProofOfWorkCheck())
-        if (block.nBits != GetNextWorkRequired(pindexPrev, &block))
+        if ((!Params().SkipProofOfWorkCheck()) &&
+           (block.nBits != GetNextWorkRequired(pindexPrev, &block)))
             return state.DoS(100, error("AcceptBlock() : incorrect proof of work"),
                              REJECT_INVALID, "bad-diffbits");
 
diff --git a/src/test/blockv2_tests.cpp b/src/test/blockv2_tests.cpp
index 0d59c8761b..96c0df5e6b 100644
--- a/src/test/blockv2_tests.cpp
+++ b/src/test/blockv2_tests.cpp
@@ -9,33 +9,34 @@
 
 #include <boost/test/unit_test.hpp>
 
-// Tests the majority rule which states that after 1000 v2 blocks no v1 block can go
+// This test cheecks the majority rule which states that after 1000 v2 blocks no new v1 block can be part of that branch.
+
 BOOST_AUTO_TEST_SUITE(blockv2_tests)
 
 static CScript scriptPubKey = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;
 
 static void SetEmptyBlock(CBlock * pblock)
 {
-        pblock->nVersion = 2;
-        pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1;
-        pblock->nNonce = 0;
+    pblock->nVersion = 2;
+    pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1;
+    pblock->nNonce = 0;
 }
 
-static void SetBlockDefaultAttributesAndHeight(CBlock * pblock,bool addHeight,int difValue)
+static void SetBlockDefaultAttributesAndHeight(CBlock * pblock,bool addHeight,int heightDifference)
 {
-        SetEmptyBlock(pblock);
+    SetEmptyBlock(pblock);
 
-        // Add the coinbase
-        CMutableTransaction txCoinbase(pblock->vtx[0]);
+    // Add the coinbase
+    CMutableTransaction txCoinbase(pblock->vtx[0]);
 
-        if (addHeight)
-            txCoinbase.vin[0].scriptSig = (CScript() << (chainActive.Height()+1+difValue) << 0);
-            else
-            txCoinbase.vin[0].scriptSig = (CScript() << difValue << 0); // At least size 2, this is a protocol spec
+    if (addHeight)
+        txCoinbase.vin[0].scriptSig = (CScript() << (chainActive.Height()+1+heightDifference) << 0);
+        else
+        txCoinbase.vin[0].scriptSig = (CScript() << heightDifference << 0); // At least size 2, this is a protocol spec
 
-        txCoinbase.vout[0].scriptPubKey = CScript();
-        pblock->vtx[0] = CTransaction(txCoinbase);
-        pblock->hashMerkleRoot = pblock->BuildMerkleTree();
+    txCoinbase.vout[0].scriptPubKey = CScript();
+    pblock->vtx[0] = CTransaction(txCoinbase);
+    pblock->hashMerkleRoot = pblock->BuildMerkleTree();
 }
 
 void CheckSubsidyHalving(CBlockTemplate * &pblocktemplate, CBlock * &pblock)
@@ -44,6 +45,8 @@ void CheckSubsidyHalving(CBlockTemplate * &pblocktemplate, CBlock * &pblock)
         {
             // The RegTest network has a low subsidy halving interval (150) so
             // we must recompute the coinbase subsidy if we reach the boundary.
+            // The unittest network allows modifying this interval. We check it so this
+            // test can work in any network.
 
             // preserve parent hash
             uint256 prevParent = pblock->hashPrevBlock;
@@ -54,6 +57,22 @@ void CheckSubsidyHalving(CBlockTemplate * &pblocktemplate, CBlock * &pblock)
         }
 }
 
+void CheckBlockAddedToBestChainSuccessfully(CBlock *pblock)
+{
+    int PreviousHeight;
+    CValidationState state;
+
+    PreviousHeight = chainActive.Height();
+    BOOST_CHECK(ProcessBlock(state, NULL, pblock));
+    BOOST_CHECK(state.IsValid());
+    BOOST_CHECK((PreviousHeight+1) == chainActive.Height()); // to differentiate from orphan blocks, which also get accepted in ProcessBlock()
+
+    // Previous checks do not assure the current best chain has pblock as tip. It could be the case that a because
+    // of a malfunction in the chain reorganization code, a reorganization causes an increase of the chain length, but with another tip.
+    // So we also check that.
+    BOOST_CHECK(chainActive.Tip()->GetBlockHash()==pblock->GetHash());
+}
+
 void Blockv2test()
 {
     assert(Params().NetworkID() == CBaseChainParams::UNITTEST);
@@ -73,16 +92,17 @@ void Blockv2test()
     uint256 hash;
     int PreviousHeight;
 
-
     LOCK(cs_main);
 
-
     // Simple block creation, nothing special yet.
     pblocktemplate = CreateNewBlock(scriptPubKey);
     CBlock *pblock = &pblocktemplate->block; // pointer for convenience
 
     LogPrintf("Blockv2test block v1 add begin\n");
-    // First create a block v1, check that it is accepted. The block has an invalid height
+
+    ////////////////////////////////////////////////////////////////////////////////////////
+    // First create a block v1, check that it is accepted. The block has an invalid height.
+    ////////////////////////////////////////////////////////////////////////////////////////
     SetBlockDefaultAttributesAndHeight(pblock,false,5000);
     pblock->nVersion = 1;
     CValidationState state1;
@@ -93,7 +113,6 @@ void Blockv2test()
     pblock->hashPrevBlock = pblock->GetHash(); // update parent
 
 
-    // Now create exactly 1000 blocks v2
 
     // First check that the supermajority threshold is exactly 1000 blocks
     BOOST_CHECK(Params().ToCheckBlockUpgradeMajority()==1000);  //
@@ -105,11 +124,19 @@ void Blockv2test()
 
     LogPrintf("Blockv2test BIP30 repetition begin\n");
 
+    ///////////////////////////////////////////////////////////////////////////////////////////////////////
     // First, if we try to add a block v2 with the same coinbase tx, we should get
     // "bad-txns-BIP30" because the coinbase tx has the same hash as the previous.
-    // Unluckily, even if ConnectBlock returns a "bad-txns-BIP30", ActivateBestChainStep clears
-    // the state, so we get true here and the "bad-txns-BIP30" reason is lost.
-    // We verify instead that the chain height has not been incremented.
+    // Even if ConnectBlock returns a "bad-txns-BIP30", ActivateBestChainStep clears
+    // the state, so we get true here and the "bad-txns-BIP30" reason is lost. But this
+    // is the intended behaviour: 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.
+    // Because we cannot access the failure reason here, we just verify instead that the chain
+    // height has not been incremented.
+    //////////////////////////////////////////////////////////////////////////////////////////////////////
 
     CValidationState state7;
     PreviousHeight = chainActive.Height();
@@ -121,6 +148,11 @@ void Blockv2test()
     BOOST_CHECK(PreviousHeight == chainActive.Height()); // we check the block has not been added.
 
     LogPrintf("Blockv2test 750 v2 blocks  begin\n");
+
+    ////////////////////////////
+    // Now create 750 v2 blocks
+    ////////////////////////////
+
     for (int i=0;i<750;i++)
     {
 
@@ -131,18 +163,17 @@ void Blockv2test()
         // We add a value to the height to make is NOT equal to the actual height.
         SetBlockDefaultAttributesAndHeight(pblock,true,1000); // blocks version 2 without height are allowed! for only 750 blocks
         pblock->nVersion = 2;
-        CValidationState state;
 
-        PreviousHeight = chainActive.Height();
-        BOOST_CHECK(ProcessBlock(state, NULL, pblock));
-        BOOST_CHECK(state.IsValid());
-        BOOST_CHECK((PreviousHeight+1) == chainActive.Height()); // to differentiate from orphan blocks, which also get accepted in ProcessBlock()
+        CheckBlockAddedToBestChainSuccessfully(pblock);
         pblock->hashPrevBlock = pblock->GetHash(); // update parent
     }
 
     LogPrintf("Blockv2test v2 without height rejected begin\n");
 
-    // Now we try to add a block v2, with an invalid height and it should be rejected. We use 2000 because is not in the range [1000..1750].
+    /////////////////////////////////////////////////////////////////////////////////////
+    // Now we try to add a block v2, with an invalid height and it should be rejected.
+    // We use 2000 as argument heightDifference because is not in the range [1000..1750].
+    /////////////////////////////////////////////////////////////////////////////////////
     CheckSubsidyHalving(pblocktemplate,pblock);
     SetBlockDefaultAttributesAndHeight(pblock,true,2000); //
     pblock->nVersion = 2;
@@ -154,8 +185,10 @@ void Blockv2test()
 
     LogPrintf("Blockv2test v2 with height accepted begin\n");
 
-
-    // Now we add a block with height, must be ok.
+    /////////////////////////////////////////////////////////////
+    // Now we add 200 additional blocks, until we get 950
+    // (the threshold where v1 blocks are not accepted anymore)
+    /////////////////////////////////////////////////////////////
     for (int i=0;i<200;i++)
     {
 
@@ -163,19 +196,16 @@ void Blockv2test()
         CheckSubsidyHalving(pblocktemplate,pblock);
         SetBlockDefaultAttributesAndHeight(pblock,true,0);
         pblock->nVersion = 2;
-        CValidationState state;
-        PreviousHeight = chainActive.Height();
-        BOOST_CHECK(ProcessBlock(state, NULL, pblock));
-        BOOST_CHECK(state.IsValid());
-        BOOST_CHECK((PreviousHeight+1) == chainActive.Height()); // to differentiate from orphan blocks, which also get accepted in ProcessBlock()
-
+        CheckBlockAddedToBestChainSuccessfully(pblock);
         pblock->hashPrevBlock = pblock->GetHash(); // update parent
     }
 
 
     LogPrintf("Blockv2test block v1 rejected\n");
-    // Now we add 200 additional blocks, until we get 950 (the threshold were v1 blocks are not accepted anymore)
+
+    /////////////////////////////////////////////////////////////////////////////////////////
     // Now we try to add a block v1, it should be rejected, even if it hash the height field
+    /////////////////////////////////////////////////////////////////////////////////////////
     CheckSubsidyHalving(pblocktemplate,pblock);
     SetBlockDefaultAttributesAndHeight(pblock,true,0);
     pblock->nVersion = 1;
@@ -185,11 +215,9 @@ void Blockv2test()
     BOOST_CHECK(state2.GetRejectReason()=="bad-version");
     // Do not update parent since block has failed
 
-
-
-    // Some other missing tests, added here as bonus...
-
+    ////////////////////////////////////////////////////////
     // Block time too old check
+    ////////////////////////////////////////////////////////
     CheckSubsidyHalving(pblocktemplate,pblock);
     SetBlockDefaultAttributesAndHeight(pblock,true,0);
     pblock->nVersion = 2;
@@ -200,7 +228,9 @@ void Blockv2test()
     BOOST_CHECK(state4.GetRejectReason()=="time-too-old");
     // Do not update parent since block has failed
 
+    ////////////////////////////////////////////////////////
     // Adding a non-final coinbase, must modify coinbase
+    ////////////////////////////////////////////////////////
     CheckSubsidyHalving(pblocktemplate,pblock);
     SetEmptyBlock(pblock);
     // Use a mutable coinbase to change nLockTime and  nSequence
@@ -217,10 +247,23 @@ void Blockv2test()
     BOOST_CHECK(state5.GetRejectReason()=="bad-txns-nonfinal");
     // Do not update parent since block has failed
 
+    ModifiableParams()->setSkipProofOfWorkCheck(false);
+
+    ////////////////////////////////////////////////////////////////////////////////
+    // Just to be sure that proof-of-work skipping is working ok, we submit a block
+    // without enought proof of work and it must be rejected.
+    ////////////////////////////////////////////////////////////////////////////////
+    CheckSubsidyHalving(pblocktemplate,pblock);
+    SetBlockDefaultAttributesAndHeight(pblock,true,0);
+    pblock->nVersion = 2;
+    CValidationState state6;
+    BOOST_CHECK(ProcessBlock(state6, NULL, pblock)==false);
+    BOOST_CHECK(!state6.IsValid());
+    BOOST_CHECK(state6.GetRejectReason()=="high-hash");
 
     delete pblocktemplate;
 
-    ModifiableParams()->setSkipProofOfWorkCheck(false);
+
     LogPrintf("Blockv2test testcase ends\n");
 }