LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Merge #13786: refactor: Avoid locking tx pool cs thrice

Browse source 
fa5ed4f8d2 refactor: Avoid locking tx pool cs thrice (MarcoFalke)

Pull request description:

  `addUnchecked` is (outside the tests) only called by ATMP, which already takes the tx pool read lock. So locking it twice more in both `addUnchecked` methods seems redundant.

  Similarly `CalculateMemPoolAncestors` is (beside once in the wallet) only called in contexts, where the tx pool lock is already taken. So remove the lock there as well.

Tree-SHA512: fcf603b570da0fc529fe6db8add218663eae52845510732bee0d4611263d2429d3d3c9c8ae68493d67287d13504500ed51905ccbe711eb15a0af3b019edad543
This commit is contained in:
MarcoFalke 2018-07-30 16:16:16 -04:00
commit 84d5a6210c
No known key found for this signature in database
GPG key ID: D2EA4850E7528B25
8 changed files with 18 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/bench/mempool_eviction.cpp
View file

@ -9,7 +9,7 @@
#include <list>
#include <vector>
static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& pool)
static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs)
{
int64_t nTime = 0;
unsigned int nHeight = 1;
@ -108,6 +108,7 @@ static void MempoolEviction(benchmark::State& state)
tx7.vout[1].nValue = 10 * COIN;
CTxMemPool pool;
LOCK(pool.cs);
// Create transaction references outside the "hot loop"
const CTransactionRef tx1_r{MakeTransactionRef(tx1)};
const CTransactionRef tx2_r{MakeTransactionRef(tx2)};

6
src/test/blockencodings_tests.cpp
View file

@ -62,8 +62,8 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
TestMemPoolEntryHelper entry;
CBlock block(BuildBlockTestCase());
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(block.vtx[2]));
LOCK(pool.cs);
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(block.vtx[2]));
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
// Do a simple ShortTxIDs RT
@ -162,8 +162,8 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
TestMemPoolEntryHelper entry;
CBlock block(BuildBlockTestCase());
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(block.vtx[2]));
LOCK(pool.cs);
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(block.vtx[2]));
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
uint256 txhash;
@ -232,8 +232,8 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
TestMemPoolEntryHelper entry;
CBlock block(BuildBlockTestCase());
pool.addUnchecked(block.vtx[1]->GetHash(), entry.FromTx(block.vtx[1]));
LOCK(pool.cs);
pool.addUnchecked(block.vtx[1]->GetHash(), entry.FromTx(block.vtx[1]));
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[1]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
uint256 txhash;

7
src/test/mempool_tests.cpp
View file

@ -55,6 +55,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
CTxMemPool testPool;
LOCK(testPool.cs);
// Nothing in pool, remove should do nothing:
unsigned int poolSize = testPool.size();
@ -119,6 +120,7 @@ static void CheckSort(CTxMemPool &pool, std::vector<std::string> &sortedOrder) E
BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
{
CTxMemPool pool;
LOCK(pool.cs);
TestMemPoolEntryHelper entry;
/* 3rd highest fee */
@ -165,7 +167,6 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
sortedOrder[2] = tx1.GetHash().ToString(); // 10000
sortedOrder[3] = tx4.GetHash().ToString(); // 15000
sortedOrder[4] = tx2.GetHash().ToString(); // 20000
LOCK(pool.cs);
CheckSort<descendant_score>(pool, sortedOrder);
/* low fee but with high fee child */
@ -292,6 +293,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
{
CTxMemPool pool;
LOCK(pool.cs);
TestMemPoolEntryHelper entry;
/* 3rd highest fee */
@ -347,7 +349,6 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
}
sortedOrder[4] = tx3.GetHash().ToString(); // 0
LOCK(pool.cs);
CheckSort<ancestor_score>(pool, sortedOrder);
/* low fee parent with high fee child */
@ -421,6 +422,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
{
CTxMemPool pool;
LOCK(pool.cs);
TestMemPoolEntryHelper entry;
CMutableTransaction tx1 = CMutableTransaction();
@ -593,6 +595,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestryTests)
size_t ancestors, descendants;
CTxMemPool pool;
LOCK(pool.cs);
TestMemPoolEntryHelper entry;
/* Base transaction */

3
src/test/miner_tests.cpp
View file

@ -99,7 +99,7 @@ static bool TestSequenceLocks(const CTransaction &tx, int flags)
// Test suite for ancestor feerate transaction selection.
// Implemented as an additional function, rather than a separate test case,
// to allow reusing the blockchain created in CreateNewBlock_validity.
static void TestPackageSelection(const CChainParams& chainparams, CScript scriptPubKey, std::vector<CTransactionRef>& txFirst)
static void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::mempool.cs)
{
// Test the ancestor feerate transaction selection.
TestMemPoolEntryHelper entry;
@ -253,6 +253,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
}
LOCK(cs_main);
LOCK(::mempool.cs);
// Just to make sure we can still make simple blocks
BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey));

1
src/test/policyestimator_tests.cpp
View file

@ -18,6 +18,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
{
CBlockPolicyEstimator feeEst;
CTxMemPool mpool(&feeEst);
LOCK(mpool.cs);
TestMemPoolEntryHelper entry;
CAmount basefee(2000);
CAmount deltaFee(100);

4
src/txmempool.cpp
View file

@ -151,8 +151,6 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashes
bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) const
{
LOCK(cs);
setEntries parentHashes;
const CTransaction &tx = entry.GetTx();
@ -363,7 +361,6 @@ void CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
// Add to memory pool without checking anything.
// Used by AcceptToMemoryPool(), which DOES do
// all the appropriate checks.
LOCK(cs);
indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
mapLinks.insert(make_pair(newit, TxLinks()));
@ -933,7 +930,6 @@ int CTxMemPool::Expire(int64_t time) {
void CTxMemPool::addUnchecked(const uint256&hash, const CTxMemPoolEntry &entry, bool validFeeEstimate)
{
LOCK(cs);
setEntries setAncestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;

6
src/txmempool.h
View file

@ -539,8 +539,8 @@ public:
// Note that addUnchecked is ONLY called from ATMP outside of tests
// and any other callers may break wallet's in-mempool tracking (due to
// lack of CValidationInterface::TransactionAddedToMempool callbacks).
void addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, bool validFeeEstimate = true);
void addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate = true);
void addUnchecked(const uint256& hash, const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
void addUnchecked(const uint256& hash, const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
void removeRecursive(const CTransaction &tx, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN);
void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags);
@ -596,7 +596,7 @@ public:
* fSearchForParents = whether to search a tx's vin for in-mempool parents, or
* look up parents from mapLinks. Must be true for entries not in the mempool
*/
bool CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents = true) const;
bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, setEntries& setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string& errString, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
/** Populate setDescendants with all in-mempool descendants of hash.
* Assumes that setDescendants includes all in-mempool descendants of anything

3
src/wallet/wallet.cpp
View file

@ -3018,7 +3018,8 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
size_t nLimitDescendants = gArgs.GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT);
size_t nLimitDescendantSize = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000;
std::string errString;
if (!mempool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
LOCK(::mempool.cs);
if (!::mempool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
strFailReason = _("Transaction has too long of a mempool chain");
return false;
}