From a99b76f26958829c2b9ca4ffa5b1d81912b8acc7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 4 Dec 2017 18:25:57 -0500 Subject: Require no cs_main lock for ProcessNewBlock/ActivateBestChain This requires the removal of some very liberal (incorrect) cs_mains sprinkled in some tests. It adds some chainActive.Tip() races, but the tests are all single-threaded anyway. --- src/test/miner_tests.cpp | 38 ++++++++++++++++++++---------------- src/test/txvalidationcache_tests.cpp | 21 ++++++++++---------- 2 files changed, 32 insertions(+), 27 deletions(-) (limited to 'src/test') diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index b1a2032ea8..f968869bf5 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -205,7 +205,6 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) entry.nFee = 11; entry.nHeight = 11; - LOCK(cs_main); fCheckpointsEnabled = false; // Simple block creation, nothing special yet: @@ -218,27 +217,32 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) for (unsigned int i = 0; i < sizeof(blockinfo)/sizeof(*blockinfo); ++i) { CBlock *pblock = &pblocktemplate->block; // pointer for convenience - pblock->nVersion = 1; - pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1; - CMutableTransaction txCoinbase(*pblock->vtx[0]); - txCoinbase.nVersion = 1; - txCoinbase.vin[0].scriptSig = CScript(); - txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce); - txCoinbase.vin[0].scriptSig.push_back(chainActive.Height()); - txCoinbase.vout.resize(1); // Ignore the (optional) segwit commitment added by CreateNewBlock (as the hardcoded nonces don't account for this) - txCoinbase.vout[0].scriptPubKey = CScript(); - pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); - if (txFirst.size() == 0) - baseheight = chainActive.Height(); - if (txFirst.size() < 4) - txFirst.push_back(pblock->vtx[0]); - pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); - pblock->nNonce = blockinfo[i].nonce; + { + LOCK(cs_main); + pblock->nVersion = 1; + pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1; + CMutableTransaction txCoinbase(*pblock->vtx[0]); + txCoinbase.nVersion = 1; + txCoinbase.vin[0].scriptSig = CScript(); + txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce); + txCoinbase.vin[0].scriptSig.push_back(chainActive.Height()); + txCoinbase.vout.resize(1); // Ignore the (optional) segwit commitment added by CreateNewBlock (as the hardcoded nonces don't account for this) + txCoinbase.vout[0].scriptPubKey = CScript(); + pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); + if (txFirst.size() == 0) + baseheight = chainActive.Height(); + if (txFirst.size() < 4) + txFirst.push_back(pblock->vtx[0]); + pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); + pblock->nNonce = blockinfo[i].nonce; + } std::shared_ptr shared_pblock = std::make_shared(*pblock); BOOST_CHECK(ProcessNewBlock(chainparams, shared_pblock, true, nullptr)); pblock->hashPrevBlock = pblock->GetHash(); } + LOCK(cs_main); + // Just to make sure we can still make simple blocks BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey)); diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index fe8cb6126e..10acabe0be 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -66,7 +66,6 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup) // Test 1: block with both of those transactions should be rejected. block = CreateAndProcessBlock(spends, scriptPubKey); - LOCK(cs_main); BOOST_CHECK(chainActive.Tip()->GetBlockHash() != block.GetHash()); // Test 2: ... and should be rejected if spend1 is in the memory pool @@ -190,12 +189,12 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) spend_tx.vin[0].scriptSig << vchSig; } - LOCK(cs_main); - // Test that invalidity under a set of flags doesn't preclude validity // under other (eg consensus) flags. // spend_tx is invalid according to DERSIG { + LOCK(cs_main); + CValidationState state; PrecomputedTransactionData ptd_spend_tx(spend_tx); @@ -213,15 +212,17 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) // test later that block validation works fine in the absence of cached // successes. ValidateCheckInputsForAllFlags(spend_tx, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_STRICTENC, false); + } - // And if we produce a block with this tx, it should be valid (DERSIG not - // enabled yet), even though there's no cache entry. - CBlock block; + // And if we produce a block with this tx, it should be valid (DERSIG not + // enabled yet), even though there's no cache entry. + CBlock block; - block = CreateAndProcessBlock({spend_tx}, p2pk_scriptPubKey); - BOOST_CHECK(chainActive.Tip()->GetBlockHash() == block.GetHash()); - BOOST_CHECK(pcoinsTip->GetBestBlock() == block.GetHash()); - } + block = CreateAndProcessBlock({spend_tx}, p2pk_scriptPubKey); + BOOST_CHECK(chainActive.Tip()->GetBlockHash() == block.GetHash()); + BOOST_CHECK(pcoinsTip->GetBestBlock() == block.GetHash()); + + LOCK(cs_main); // Test P2SH: construct a transaction that is valid without P2SH, and // then test validity with P2SH. -- cgit v1.2.3 From 36137497f1e2b3324ca84550f4f295dcd605d1fa Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 4 Dec 2017 18:57:55 -0500 Subject: Block ActivateBestChain to empty validationinterface queue --- src/test/test_bitcoin.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/test') diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index f52c8ccc21..6bd228a63a 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -69,9 +69,9 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha fs::create_directories(pathTemp); gArgs.ForceSetArg("-datadir", pathTemp.string()); - // Note that because we don't bother running a scheduler thread here, - // callbacks via CValidationInterface are unreliable, but that's OK, - // our unit tests aren't testing multiple parts of the code at once. + // We have to run a scheduler thread to prevent ActivateBestChain + // from blocking due to queue overrun. + threadGroup.create_thread(boost::bind(&CScheduler::serviceQueue, &scheduler)); GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); mempool.setSanityCheck(1.0); -- cgit v1.2.3