txmempool: split epoch logic into class #18017
Conversation
|
Concept ACK. Will need to play around with it a little bit to test that clang actually prevents compiling incorrect uses. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
Concept ACK, nice |
|
Concept ACK. |
hebasto
left a comment
There was a problem hiding this comment.
A brilliant idea to leverage the Clang Thread Safety Analysis!
|
Incorporated @hebasto's suggested changes |
hebasto
left a comment
There was a problem hiding this comment.
ACK 837d4e4f5aeb2f110143c59b7dd05eac6ae52b63, tested on Linux Mint 19.3: the Clang's thread safety annotations indeed reduce chances of the Epoch class misuse (verified by different code manipulations).
May I suggest two additional annotations:
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -107,6 +107,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan
void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashesToUpdate)
{
AssertLockHeld(cs);
+ AssertLockNotHeld(m_epoch);
// For each entry in vHashesToUpdate, store the set of in-mempool, but not
// in-vHashesToUpdate transactions, so that we don't have to recalculate
// descendants when we come across a previously seen entry.
diff --git a/src/txmempool.h b/src/txmempool.h
index 655afc3b8..b72ad229c 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -797,7 +797,7 @@ private:
*/
void UpdateForDescendants(txiter updateIt,
cacheMap &cachedDescendants,
- const std::set<uint256> &setExclude) EXCLUSIVE_LOCKS_REQUIRED(cs);
+ const std::set<uint256> &setExclude) EXCLUSIVE_LOCKS_REQUIRED(cs) LOCKS_EXCLUDED(m_epoch);
/** Update ancestors of hash to add/remove it as a descendant transaction. */
void UpdateAncestorsOf(bool add, txiter hash, setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs);
/** Set ancestor state for an entry */?
837d4e4 to
c95663c
Compare
|
Rebased |
|
Rebased to deal with adjacent lines changed in #19935 |
hebasto
left a comment
There was a problem hiding this comment.
ACK 88019f9a183d396713fd357604a6e472c5ed8807
It seems both CTxMemPool::visited member functions could be private.
|
Yes this is correct; IIRC there's some stalled out patches which require them to be visible :) |
There was a problem hiding this comment.
ACK fd6580e using clang thread safety annotations looks like a very good idea, and the encapsulation this change adds should improve robustness (and possible unit test-ability) of the code. Verified that changing some of the locking duly provoked build-time warnings with Clang 9 on Debian and that small changes in the new Epoch class were covered by failing functional test assertions in mempool_updatefromblock.py, mempool_resurrect.py, and mempool_reorg.py
example clang warning
/txmempool.h:840:24: warning: calling function 'visited' requires holding mutex 'm_epoch' exclusively [-Wthread-safety-analysis]
return m_epoch.visited(it->m_epoch_marker);
Ignore the two comments below unless you need to retouch anything (the documentation was move-only anyway).
| * adds an asymptotic factor of O(log n) to traversals cost and triggers O(n) | ||
| * more dynamic memory allocations. | ||
| * In many algorithms we can replace std::set with an internal mempool | ||
| * counter to track the time (or, "epoch") that we began a traversal, and |
There was a problem hiding this comment.
| * counter to track the time (or, "epoch") that we began a traversal, and | |
| * counter to track the time (or, "epoch") that we began a traversal and |
| * Algorithms using std::set can be replaced on a one by one basis. | ||
| * Both techniques are not fundamentally incompatible across the codebase. | ||
| * Generally speaking, however, the remaining use of std::set for mempool | ||
| * traversal should be viewed as a TODO for replacement with an epoch based |
There was a problem hiding this comment.
| * traversal should be viewed as a TODO for replacement with an epoch based | |
| * traversal should be viewed as a TODO for replacement with an epoch-based |
Summary: This is a backport of [[bitcoin/bitcoin#18017 | core#18017]] Test Plan: With clang and debug: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11308
Splits the epoch logic introduced in #17925 into a separate class.
Uses clang's thread safety annotations and encapsulates the data more strongly to reduce chances of bugs from API misuse.