This PR is related to #19303 and gets rid of the RecursiveMutex m_most_recent_block_mutex. All of the critical sections (5 in total) only directly access the guarded elements, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex:
The scope of the last critical section is reduced in the first commit, in order to avoid calling the non-trivial method CConnman::PushMessage while the lock is held.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Concept ACK. Thanks!
4703 | @@ -4704,7 +4704,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4704 | 4705 | bool fGotBlockFromCache = false; 4706 | { 4707 | - LOCK(cs_most_recent_block); 4708 | + LOCK(most_recent_block_mutex);
... it is not possible that within one section another one is called...
It is not obvious where functions with a non-trivial body--like PushMessage--are called. Even everything is correct now, there is a possibility to introduce a recursion in the future by modifying the PushMessage function or other functions been called from within it.
Could we just call PushMessage without holding most_recent_block_mutex?
Fair enough, will take a deeper look.
Approach ACK cac83660c07568820dcd0cd9cc71548bf05857c3.
Added another commit which reduces the scope of the lock most_recent_block_mutex, as suggested by [@hebasto](/github-metadata-backup-bitcoin-bitcoin/contributor/hebasto/). Rather than calling the non-trivial method CConnman::PushMessage immediately, only the (cached) cmpctblock message is now assembled and sent after, outside of the critical section.
Rebased on master. Note that the scripted-diff renaming commit (getting rid of the lock's cs_ prefix) was dropped as this was already resolved in the recently merged PR #24543. Also updated the PR title and description accordingly with the current critical section code snippets.
This avoids calling the non-trivial method
`CConnman::PushMessage` within the critical section.
In each of the critical sections, only the the guarded variables are
accessed, without any chance that within one section another one is
called. Hence, we can use an ordinary Mutex instead of RecursiveMutex.
Rebased on master, updated the PR description with the current critical section code snippets (shorter after #20799 has been merged). The boolean variable fGotBlockFromCache is now removed in the first commit and a std::optional on the cached message is used instead, which I think is a bit more elegant.
4769 | + cached_cmpctblock_msg = msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block); 4770 | } 4771 | } 4772 | - if (!fGotBlockFromCache) { 4773 | + if (cached_cmpctblock_msg.has_value()) { 4774 | + m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value()));
As std::optional default value is nullopt. Could write it as:
if (cached_cmpctblock_msg) {
m_connman.PushMessage(pto, std::move(*cached_cmpctblock_msg));
}
Just realized, all of this could be written as:
if (WITH_LOCK(m_most_recent_block_mutex, return m_most_recent_block_hash == pBestIndex->GetBlockHash())) {
m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block));
m_most_recent_compact_block should also be accessed from within a critical section.
Code ACK 83003ffe with a small comment.
ACK 83003ffe049a432f6fa4127e054f073127e70b90
