Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove txConflicted #9240

Merged
merged 2 commits into from Dec 10, 2016
Merged

Remove txConflicted #9240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf663f8
remove external usage of mempool conflict tracking
morcos Nov 29, 2016
a874ab5
remove internal tracking of mempool conflicts for reporting to wallet
morcos Nov 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/test/blockencodings_tests.cpp
View file
Open in desktop
Expand Up @@ -80,9 +80,9 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)

BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);

std::vector<CTransactionRef> removed;
pool.removeRecursive(*block.vtx[2], &removed);
BOOST_CHECK_EQUAL(removed.size(), 1);
size_t poolSize = pool.size();
pool.removeRecursive(*block.vtx[2]);
BOOST_CHECK_EQUAL(pool.size(), poolSize - 1);

CBlock block2;
std::vector<CTransactionRef> vtx_missing;
Expand Down
42 changes: 22 additions & 20 deletions src/test/mempool_tests.cpp
View file
Open in desktop
Expand Up @@ -55,17 +55,17 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)


CTxMemPool testPool(CFeeRate(0));
std::vector<CTransactionRef> removed;

// Nothing in pool, remove should do nothing:
testPool.removeRecursive(txParent, &removed);
BOOST_CHECK_EQUAL(removed.size(), 0);
unsigned int poolSize = testPool.size();
testPool.removeRecursive(txParent);
BOOST_CHECK_EQUAL(testPool.size(), poolSize);

// Just the parent:
testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent));
testPool.removeRecursive(txParent, &removed);
BOOST_CHECK_EQUAL(removed.size(), 1);
removed.clear();
poolSize = testPool.size();
testPool.removeRecursive(txParent);
BOOST_CHECK_EQUAL(testPool.size(), poolSize - 1);

// Parent, children, grandchildren:
testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent));
Expand All @@ -75,19 +75,21 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
testPool.addUnchecked(txGrandChild[i].GetHash(), entry.FromTx(txGrandChild[i]));
}
// Remove Child[0], GrandChild[0] should be removed:
testPool.removeRecursive(txChild[0], &removed);
BOOST_CHECK_EQUAL(removed.size(), 2);
removed.clear();
poolSize = testPool.size();
testPool.removeRecursive(txChild[0]);
BOOST_CHECK_EQUAL(testPool.size(), poolSize - 2);
// ... make sure grandchild and child are gone:
testPool.removeRecursive(txGrandChild[0], &removed);
BOOST_CHECK_EQUAL(removed.size(), 0);
testPool.removeRecursive(txChild[0], &removed);
BOOST_CHECK_EQUAL(removed.size(), 0);
poolSize = testPool.size();
testPool.removeRecursive(txGrandChild[0]);
BOOST_CHECK_EQUAL(testPool.size(), poolSize);
poolSize = testPool.size();
testPool.removeRecursive(txChild[0]);
BOOST_CHECK_EQUAL(testPool.size(), poolSize);
// Remove parent, all children/grandchildren should go:
testPool.removeRecursive(txParent, &removed);
BOOST_CHECK_EQUAL(removed.size(), 5);
poolSize = testPool.size();
testPool.removeRecursive(txParent);
BOOST_CHECK_EQUAL(testPool.size(), poolSize - 5);
BOOST_CHECK_EQUAL(testPool.size(), 0);
removed.clear();

// Add children and grandchildren, but NOT the parent (simulate the parent being in a block)
for (int i = 0; i < 3; i++)
Expand All @@ -97,10 +99,10 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
}
// Now remove the parent, as might happen if a block-re-org occurs but the parent cannot be
// put into the mempool (maybe because it is non-standard):
testPool.removeRecursive(txParent, &removed);
BOOST_CHECK_EQUAL(removed.size(), 6);
poolSize = testPool.size();
testPool.removeRecursive(txParent);
BOOST_CHECK_EQUAL(testPool.size(), poolSize - 6);
BOOST_CHECK_EQUAL(testPool.size(), 0);
removed.clear();
}

template<typename name>
Expand Down Expand Up @@ -412,7 +414,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
/* after tx6 is mined, tx7 should move up in the sort */
std::vector<CTransactionRef> vtx;
vtx.push_back(MakeTransactionRef(tx6));
pool.removeForBlock(vtx, 1, NULL, false);
pool.removeForBlock(vtx, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the fCurrentEstimate parameter value from false to true. Intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there was no reason for that to be set. It's only for fee estimates which are useless in this test.


sortedOrder.erase(sortedOrder.begin()+1);
sortedOrder.pop_back();
Expand Down
15 changes: 5 additions & 10 deletions src/txmempool.cpp
View file
Open in desktop
Expand Up @@ -503,7 +503,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants
}
}

void CTxMemPool::removeRecursive(const CTransaction &origTx, std::vector<CTransactionRef>* removed)
void CTxMemPool::removeRecursive(const CTransaction &origTx)
{
// Remove transaction from memory pool
{
Expand All @@ -530,11 +530,6 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, std::vector<CTransa
BOOST_FOREACH(txiter it, txToRemove) {
CalculateDescendants(it, setAllRemoves);
}
if (removed) {
BOOST_FOREACH(txiter it, setAllRemoves) {
removed->emplace_back(it->GetSharedTx());
}
}
RemoveStaged(setAllRemoves, false);
}
}
Expand Down Expand Up @@ -576,7 +571,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
RemoveStaged(setAllRemoves, false);
}

void CTxMemPool::removeConflicts(const CTransaction &tx, std::vector<CTransactionRef>* removed)
void CTxMemPool::removeConflicts(const CTransaction &tx)
{
// Remove transactions which depend on inputs of tx, recursively
LOCK(cs);
Expand All @@ -586,7 +581,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx, std::vector<CTransactio
const CTransaction &txConflict = *it->second;
if (txConflict != tx)
{
removeRecursive(txConflict, removed);
removeRecursive(txConflict);
ClearPrioritisation(txConflict.GetHash());
}
}
Expand All @@ -597,7 +592,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx, std::vector<CTransactio
* Called when a block is connected. Removes from mempool and updates the miner fee estimator.
*/
void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight,
std::vector<CTransactionRef>* conflicts, bool fCurrentEstimate)
bool fCurrentEstimate)
{
LOCK(cs);
std::vector<CTxMemPoolEntry> entries;
Expand All @@ -617,7 +612,7 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
stage.insert(it);
RemoveStaged(stage, true);
}
removeConflicts(*tx, conflicts);
removeConflicts(*tx);
ClearPrioritisation(tx->GetHash());
}
// After the txs in the new block have been removed from the mempool, update policy estimates
Expand Down
6 changes: 3 additions & 3 deletions src/txmempool.h
View file
Open in desktop
Expand Up @@ -527,11 +527,11 @@ class CTxMemPool
bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, bool fCurrentEstimate = true);
bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true);

void removeRecursive(const CTransaction &tx, std::vector<CTransactionRef>* removed = NULL);
void removeRecursive(const CTransaction &tx);
void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags);
void removeConflicts(const CTransaction &tx, std::vector<CTransactionRef>* removed = NULL);
void removeConflicts(const CTransaction &tx);
void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight,
std::vector<CTransactionRef>* conflicts = NULL, bool fCurrentEstimate = true);
bool fCurrentEstimate = true);
void clear();
void _clear(); //lock free
bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb);
Expand Down
12 changes: 3 additions & 9 deletions src/validation.cpp
View file
Open in desktop
Expand Up @@ -2153,11 +2153,10 @@ static int64_t nTimeChainState = 0;
static int64_t nTimePostConnect = 0;

/**
* Used to track conflicted transactions removed from mempool and transactions
* applied to the UTXO state as a part of a single ActivateBestChainStep call.
* Used to track blocks whose transactions were applied to the UTXO state as a
* part of a single ActivateBestChainStep call.
*/
struct ConnectTrace {
std::vector<CTransactionRef> txConflicted;
std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also get rid of the ConnectTrace struct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt was in favor of keeping it for now. I have no objection.

};

Expand Down Expand Up @@ -2208,7 +2207,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4;
LogPrint("bench", " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001);
// Remove conflicting transactions from the mempool.;
mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, &connectTrace.txConflicted, !IsInitialBlockDownload());
mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, !IsInitialBlockDownload());
// Update chainActive & related variables.
UpdateTip(pindexNew, chainparams);

Expand Down Expand Up @@ -2433,11 +2432,6 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,

// throw all transactions though the signal-interface
// while _not_ holding the cs_main lock
for (const auto& tx : connectTrace.txConflicted)
{
GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
}
// ... and about transactions that got confirmed:
for (const auto& pair : connectTrace.blocksConnected) {
assert(pair.second);
const CBlock& block = *(pair.second);
Expand Down