First 2 commits avoid unlock/lock mempool.cs and cs_main interchangeably by turning a loop in two loops - each mutex is locked throughout each corresponding loop.
Then explicit lock in CTxMemPool::RemoveUnbroadcastTx, CTxMemPool::GetUnbroadcastTxs and CTxMemPool::exists is removed forcing just 3 explicit WITH_LOCK where exists() is called and just 1 where GetUnbroadcastTxs() is called. This can be improved by adding an auxiliary function that locks and calls the original.
896- if (m_mempool.exists(elem.first)) {
897- LOCK(cs_main);
898+ std::vector<std::pair<uint256, uint256>> relay_transactions;
899+ {
900+ LOCK(m_mempool.cs);
901+ std::map<uint256, uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
GetUnbroadcastTxs and exists.
906+ {
907+ LOCK(cs_main);
908+ for (const auto& elem : relay_transactions) {
909+ RelayTransaction(elem.first, elem.second, m_connman);
910+ }
911+ }
::cs_main uninterruptible locking?
RelayTransaction doesn’t hold cs_main that long.
The changes in PeerManager::ReattemptInitialBroadcast() will execute CTxMemPool::GetUnbroadcastTxs() and CTxMemPool::exists() under CTxMemPool::cs whereas previously they were not called under this mutex. Both methods acquire the mutex themselves. So this adds more recursive mutex locks.
What is the purpose of this patch? There is no description and commit messages are a bit scarce, missing answer to “Why are we doing this?”.
420@@ -421,7 +421,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
421 for (const CTxIn& txin : it->GetTx().vin)
422 mapNextTx.erase(txin.prevout);
423
424- RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ );
425+ WITH_LOCK(cs, RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ ));
return?
915 RelayTransaction(elem.first, elem.second, m_connman);
916- } else {
917- m_mempool.RemoveUnbroadcastTx(elem.first, true);
918 }
919 }
920
This can be simplified to:
0void PeerManager::ReattemptInitialBroadcast(CScheduler& scheduler) const
1{
2 for (const auto& elem : WITH_LOCK(m_mempool.cs, return m_mempool.GetUnbroadcastTxs())) {
3 LOCK(cs_main);
4 RelayTransaction(elem.first, elem.second, m_connman);
5 }
6
7 // Schedule next run for 10-15 minutes in the future.
8 ...
9}
Because m_mempool.exists() will always return true for a tx returned by m_mempool.GetUnbroadcastTxs() if we don’t release m_mempool.cs between the two calls.
Also, the tx could be removed after we release m_mempool.cs and before we call RelayTransaction() and this is ok and is handled just fine.
cc @gzhao408
cs_main lock in each iteration?
Since it was locked in each iteration before this PR, the question should rather be “If we move LOCK(cs_main) before the loop, why would we do that?”
The frequent lock/unlock allows other threads to proceed. I don’t see a reason to change it.
The frequent lock/unlock allows other threads to proceed. I don’t see a reason to change it.
Agree (https://github.com/bitcoin/bitcoin/pull/19917#discussion_r485047637).
@vasild I don’t agree with that. Other threads can proceed but the current thread will wait unnecessarily in each iteration for the lock and as such other things will be delayed, not mentioning the mutex overhead. See https://stackoverflow.com/a/3652428.
In this case RelayTransaction is pretty quick, nothing that can potentially cause a big lock on cs_main.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.