Browse Source

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.
0.16
Matt Corallo 7 years ago
parent
commit
a99b76f269
  1. 38
      src/test/miner_tests.cpp
  2. 21
      src/test/txvalidationcache_tests.cpp
  3. 3
      src/validation.cpp
  4. 17
      src/wallet/test/wallet_tests.cpp

38
src/test/miner_tests.cpp

@ -205,7 +205,6 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
entry.nFee = 11; entry.nFee = 11;
entry.nHeight = 11; entry.nHeight = 11;
LOCK(cs_main);
fCheckpointsEnabled = false; fCheckpointsEnabled = false;
// Simple block creation, nothing special yet: // 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) for (unsigned int i = 0; i < sizeof(blockinfo)/sizeof(*blockinfo); ++i)
{ {
CBlock *pblock = &pblocktemplate->block; // pointer for convenience CBlock *pblock = &pblocktemplate->block; // pointer for convenience
pblock->nVersion = 1; {
pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1; LOCK(cs_main);
CMutableTransaction txCoinbase(*pblock->vtx[0]); pblock->nVersion = 1;
txCoinbase.nVersion = 1; pblock->nTime = chainActive.Tip()->GetMedianTimePast()+1;
txCoinbase.vin[0].scriptSig = CScript(); CMutableTransaction txCoinbase(*pblock->vtx[0]);
txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce); txCoinbase.nVersion = 1;
txCoinbase.vin[0].scriptSig.push_back(chainActive.Height()); txCoinbase.vin[0].scriptSig = CScript();
txCoinbase.vout.resize(1); // Ignore the (optional) segwit commitment added by CreateNewBlock (as the hardcoded nonces don't account for this) txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce);
txCoinbase.vout[0].scriptPubKey = CScript(); txCoinbase.vin[0].scriptSig.push_back(chainActive.Height());
pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); txCoinbase.vout.resize(1); // Ignore the (optional) segwit commitment added by CreateNewBlock (as the hardcoded nonces don't account for this)
if (txFirst.size() == 0) txCoinbase.vout[0].scriptPubKey = CScript();
baseheight = chainActive.Height(); pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase));
if (txFirst.size() < 4) if (txFirst.size() == 0)
txFirst.push_back(pblock->vtx[0]); baseheight = chainActive.Height();
pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); if (txFirst.size() < 4)
pblock->nNonce = blockinfo[i].nonce; txFirst.push_back(pblock->vtx[0]);
pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
pblock->nNonce = blockinfo[i].nonce;
}
std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock); std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
BOOST_CHECK(ProcessNewBlock(chainparams, shared_pblock, true, nullptr)); BOOST_CHECK(ProcessNewBlock(chainparams, shared_pblock, true, nullptr));
pblock->hashPrevBlock = pblock->GetHash(); pblock->hashPrevBlock = pblock->GetHash();
} }
LOCK(cs_main);
// Just to make sure we can still make simple blocks // Just to make sure we can still make simple blocks
BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey)); BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey));

21
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. // Test 1: block with both of those transactions should be rejected.
block = CreateAndProcessBlock(spends, scriptPubKey); block = CreateAndProcessBlock(spends, scriptPubKey);
LOCK(cs_main);
BOOST_CHECK(chainActive.Tip()->GetBlockHash() != block.GetHash()); BOOST_CHECK(chainActive.Tip()->GetBlockHash() != block.GetHash());
// Test 2: ... and should be rejected if spend1 is in the memory pool // 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; spend_tx.vin[0].scriptSig << vchSig;
} }
LOCK(cs_main);
// Test that invalidity under a set of flags doesn't preclude validity // Test that invalidity under a set of flags doesn't preclude validity
// under other (eg consensus) flags. // under other (eg consensus) flags.
// spend_tx is invalid according to DERSIG // spend_tx is invalid according to DERSIG
{ {
LOCK(cs_main);
CValidationState state; CValidationState state;
PrecomputedTransactionData ptd_spend_tx(spend_tx); 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 // test later that block validation works fine in the absence of cached
// successes. // successes.
ValidateCheckInputsForAllFlags(spend_tx, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_STRICTENC, false); 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 // And if we produce a block with this tx, it should be valid (DERSIG not
// enabled yet), even though there's no cache entry. // enabled yet), even though there's no cache entry.
CBlock block; CBlock block;
block = CreateAndProcessBlock({spend_tx}, p2pk_scriptPubKey); block = CreateAndProcessBlock({spend_tx}, p2pk_scriptPubKey);
BOOST_CHECK(chainActive.Tip()->GetBlockHash() == block.GetHash()); BOOST_CHECK(chainActive.Tip()->GetBlockHash() == block.GetHash());
BOOST_CHECK(pcoinsTip->GetBestBlock() == block.GetHash()); BOOST_CHECK(pcoinsTip->GetBestBlock() == block.GetHash());
}
LOCK(cs_main);
// Test P2SH: construct a transaction that is valid without P2SH, and // Test P2SH: construct a transaction that is valid without P2SH, and
// then test validity with P2SH. // then test validity with P2SH.

3
src/validation.cpp

@ -2559,6 +2559,7 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
// far from a guarantee. Things in the P2P/RPC will often end up calling // far from a guarantee. Things in the P2P/RPC will often end up calling
// us in the middle of ProcessNewBlock - do not assume pblock is set // us in the middle of ProcessNewBlock - do not assume pblock is set
// sanely for performance or correctness! // sanely for performance or correctness!
AssertLockNotHeld(cs_main);
CBlockIndex *pindexMostWork = nullptr; CBlockIndex *pindexMostWork = nullptr;
CBlockIndex *pindexNewTip = nullptr; CBlockIndex *pindexNewTip = nullptr;
@ -3383,6 +3384,8 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool *fNewBlock) bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool *fNewBlock)
{ {
AssertLockNotHeld(cs_main);
{ {
CBlockIndex *pindex = nullptr; CBlockIndex *pindex = nullptr;
if (fNewBlock) *fNewBlock = false; if (fNewBlock) *fNewBlock = false;

17
src/wallet/test/wallet_tests.cpp

@ -370,8 +370,6 @@ static void AddKey(CWallet& wallet, const CKey& key)
BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
{ {
LOCK(cs_main);
// Cap last block file size, and mine new block in a new block file. // Cap last block file size, and mine new block in a new block file.
CBlockIndex* const nullBlock = nullptr; CBlockIndex* const nullBlock = nullptr;
CBlockIndex* oldTip = chainActive.Tip(); CBlockIndex* oldTip = chainActive.Tip();
@ -379,6 +377,8 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
CBlockIndex* newTip = chainActive.Tip(); CBlockIndex* newTip = chainActive.Tip();
LOCK(cs_main);
// Verify ScanForWalletTransactions picks up transactions in both the old // Verify ScanForWalletTransactions picks up transactions in both the old
// and new block files. // and new block files.
{ {
@ -447,8 +447,6 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
// than or equal to key birthday. // than or equal to key birthday.
BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
{ {
LOCK(cs_main);
// Create two blocks with same timestamp to verify that importwallet rescan // Create two blocks with same timestamp to verify that importwallet rescan
// will pick up both blocks, not just the first. // will pick up both blocks, not just the first.
const int64_t BLOCK_TIME = chainActive.Tip()->GetBlockTimeMax() + 5; const int64_t BLOCK_TIME = chainActive.Tip()->GetBlockTimeMax() + 5;
@ -462,6 +460,8 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
SetMockTime(KEY_TIME); SetMockTime(KEY_TIME);
coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
LOCK(cs_main);
// Import key into wallet and call dumpwallet to create backup file. // Import key into wallet and call dumpwallet to create backup file.
{ {
CWallet wallet; CWallet wallet;
@ -627,10 +627,15 @@ public:
BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy)); BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy));
CValidationState state; CValidationState state;
BOOST_CHECK(wallet->CommitTransaction(wtx, reservekey, nullptr, state)); BOOST_CHECK(wallet->CommitTransaction(wtx, reservekey, nullptr, state));
CMutableTransaction blocktx;
{
LOCK(wallet->cs_wallet);
blocktx = CMutableTransaction(*wallet->mapWallet.at(wtx.GetHash()).tx);
}
CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
LOCK(wallet->cs_wallet); LOCK(wallet->cs_wallet);
auto it = wallet->mapWallet.find(wtx.GetHash()); auto it = wallet->mapWallet.find(wtx.GetHash());
BOOST_CHECK(it != wallet->mapWallet.end()); BOOST_CHECK(it != wallet->mapWallet.end());
CreateAndProcessBlock({CMutableTransaction(*it->second.tx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
it->second.SetMerkleBranch(chainActive.Tip(), 1); it->second.SetMerkleBranch(chainActive.Tip(), 1);
return it->second; return it->second;
} }
@ -641,7 +646,6 @@ public:
BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
{ {
std::string coinbaseAddress = coinbaseKey.GetPubKey().GetID().ToString(); std::string coinbaseAddress = coinbaseKey.GetPubKey().GetID().ToString();
LOCK2(cs_main, wallet->cs_wallet);
// Confirm ListCoins initially returns 1 coin grouped under coinbaseKey // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey
// address. // address.
@ -669,6 +673,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
BOOST_CHECK_EQUAL(available.size(), 2); BOOST_CHECK_EQUAL(available.size(), 2);
for (const auto& group : list) { for (const auto& group : list) {
for (const auto& coin : group.second) { for (const auto& coin : group.second) {
LOCK(wallet->cs_wallet);
wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i)); wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i));
} }
} }

Loading…
Cancel
Save