m_most_recent_block_mutex with Mutex
#24062
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
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?
most_recent_block_mutex, [as suggested by @hebasto](https://github.com/bitcoin/bitcoin/pull/24062#discussion_r784643490). 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.
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.
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:
0if (cached_cmpctblock_msg) {
1 m_connman.PushMessage(pto, std::move(*cached_cmpctblock_msg));
2}
Just realized, all of this could be written as:
0if (WITH_LOCK(m_most_recent_block_mutex, return m_most_recent_block_hash == pBestIndex->GetBlockHash())) {
1 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.
