refactor: Keep mempool interface in validation #19629
Conversation
maflcko
changed the title
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Concept ACK, and code change looks good.
Would be nice to have a case with no mempool, even to ease review. Do you have a followup with this?
|
Hmm, I think it is hard to write a meaningful test case right now. Locally I tested with diff --git a/src/init.cpp b/src/init.cpp
index 50d25e672e..7fb8b3aaa7 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1563,7 +1563,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
const int64_t load_block_index_start_time = GetTimeMillis();
try {
LOCK(cs_main);
- chainman.InitializeChainstate(node.mempool);
+ chainman.InitializeChainstate(nullptr);
chainman.m_total_coinstip_cache = nCoinCacheUsage;
chainman.m_total_coinsdb_cache = nCoinDBCache;
Though, if you have a test commit, I am happy to cherry-pick it here. |
|
I think it would be less problematic if you just pass the mempool reference (not pointer) and defer the optional change to when it's needed - |
|
That was my initial draft, but it would also mean that all changed lines will need to change again. Is there any reason to increase the |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
darosior
left a comment
darosior
left a comment
There was a problem hiding this comment.
ACK faab637301a336c318ea8ee0187a74a2c7a92ef3
src/validation.cpp
Outdated
There was a problem hiding this comment.
Locally it wouldn't compile for me with clang as soon as I remove it
There was a problem hiding this comment.
The AssertLockHeld here is silencing the compile time thread safety check, and will need to be LockAssertion lock(m_mempool->cs) after #19668 . It's not redundant with the earlier assertion, because the silencing is scoped to the block, which ends immediately for the if (m_mempool) AssertLockHeld(m_mempool->cs); line.
|
Concept ACK |
I totally agree with this. If the intention is to eventually allow |
|
@jnewbery would you drop edit: @MarcoFalke sorry for being pedantic, feel free to ignore. |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Code review ACK faab637301a336c318ea8ee0187a74a2c7a92ef3
@jnewbery would you drop if (m_mempool) checks?
I think it's fine like this. My alternative approach would have been to leave those out and keep the logic the same, but add an assert m_mempool != nullptr to the constructor, but it's not a hard review, so I think it's fine to change the logic now.
src/validation.cpp
Outdated
There was a problem hiding this comment.
For those scratching their heads over whether LOCK(nullptr) is ok, I think it's fine. It constructs a UniqueLock using this constructor:
Line 163 in a787428
Which calls the default constructor (1 in https://en.cppreference.com/w/cpp/thread/unique_lock/unique_lock) for the base std::unique_lock<Mutex> class and then exits:
Line 165 in a787428
In the destructor, the call to owns_lock() returns false (since the unique_lock doesn't have a locked mutex):
Lines 174 to 178 in a787428
and the base class destructor does nothing.
Same thing here. But it's fine as it is. |
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
Concept ACK.
Could conditional locking stuff hinder effective usage of Thread Safety Analysis (especially when CTxMemPool::cs will transit from RecursiveMutex to Mutex)?
|
Without a mempool, the mempool lock won't exist and thus can not be taken. Though, this shouldn't affect anything outside of validation. E.g. changing |
I guess it won't work. For example, this patch --- a/src/validation.h
+++ b/src/validation.h
@@ -645,7 +645,7 @@ public:
CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
// Apply the effects of a block disconnection on the UTXO set.
- bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+ bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
// Manual block validity manipulation:
bool PreciousBlock(BlockValidationState& state, const CChainParams& params, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);causes I think a such disadvantage should be avoided. |
Is such an annotation in the header file dereferencing a pointer that may be |
Not sure about annotation validity, but Clang accepts it. I don't want to lose ability to use |
ajtowns
left a comment
ajtowns
left a comment
There was a problem hiding this comment.
If we want to support not having a mempool, it might be better to virtualize CTxMemPool, so that validation expects a TxMemPoolInterface that has a cs lock to let thread safety annotations all make sense, and a bunch of virtual functions that are either implemented by CTxMemPool as is, or by dummy functions in a NoMemPool class.
If this PR just had m_mempool as a reference rather than a pointer, that would be a bunch of changes when supporting an optional mempool, though I think it would mostly be limited to the class definitions and the function arguments, which is maybe slightly less than every call site.
The conditional locking in the current patch seems pretty fragile to me, but not wrong at least.
src/validation.cpp
Outdated
There was a problem hiding this comment.
The AssertLockHeld here is silencing the compile time thread safety check, and will need to be LockAssertion lock(m_mempool->cs) after #19668 . It's not redundant with the earlier assertion, because the silencing is scoped to the block, which ends immediately for the if (m_mempool) AssertLockHeld(m_mempool->cs); line.
Sounds like a nice approach. |
maflcko
changed the title
|
Trying to test thread safety annotations with the following patch: --- a/src/interfaces/txmempool.cpp
+++ b/src/interfaces/txmempool.cpp
@@ -34,7 +34,6 @@ public:
void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int block_height) override EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
{
- AssertLockHeld(m_txmempool.cs);
m_txmempool.removeForBlock(vtx, block_height);
}
and got: Could be related to https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis |
|
Another test of thread safety annotations with the following patch: --- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2595,7 +2595,6 @@ public:
bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
{
AssertLockHeld(cs_main);
- AssertLockHeld(m_txmempool->m_mutex);
assert(pindexNew->pprev == m_chain.Tip());
// Read block from disk.
--- a/src/validation.h
+++ b/src/validation.h
@@ -689,7 +689,7 @@ public:
private:
bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_txmempool->m_mutex);
- bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_txmempool->m_mutex);
+ bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);does not raise a thread safety warning as one could expect due to this annotation: |
|
Addressed feedback by @hebasto |
|
Tested fa1d3da with #19629 (comment). Still fail. |
|
Tested fa1d3da with #19629 (comment). Looks ok: |
|
FWIW, I found a patch set from #19668 very useful to test correctness of thread safety annotations. fa1d3da + #19668 |
|
Yes, if #19668 is merged, the AsserLockHeld in
I don't think this works. Or at least it would lead to the initial solution I had (keep a mempool pointer in validation). I am happy to revert to the initial solution or take over a patch, if you have one that compiles on clang with and without --enable-debug |
Mind looking at https://github.com/hebasto/bitcoin/commits/pr19629-0816-FORK ? The same branch, rebased on top of the master (1bc8e8e) + #19668:
UPDATE: the improved patch version is available. |
|
@MarcoFalke
The same branch, rebased on top of the master (e6e277f) + #19668:: |
|
Thanks for the review everyone. Though, I think this is getting a bit out of hand and trying to do too much things in one go. Closing for now. |
|
I really thought the original implementation was fine and is an improvement over the current code. Were there actually any problems with it, or is this just a case of the best being the enemy of good and us not making a good change because it's not perfect? |
|
Opened alternative in #19826 . The mempool interface idea can be explored later, if needed. |
9 participants
Next step toward #19556
Instead of relying on the mempool global, each chainstate is given an optional mempool to keep up to date with the tip (block connections, disconnections, reorgs, ...)