refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex #24062

theStack · 2022-01-14T01:53:43Z

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:

bitcoin/src/net_processing.cpp

Lines 1650 to 1655 in b019cdc

    
           { 
        
               LOCK(m_most_recent_block_mutex); 
        
               m_most_recent_block_hash = hashBlock; 
        
               m_most_recent_block = pblock; 
        
               m_most_recent_compact_block = pcmpctblock; 
        
           }

bitcoin/src/net_processing.cpp

Lines 1861 to 1865 in b019cdc

    
           { 
        
               LOCK(m_most_recent_block_mutex); 
        
               a_recent_block = m_most_recent_block; 
        
               a_recent_compact_block = m_most_recent_compact_block; 
        
           }

bitcoin/src/net_processing.cpp

Lines 3149 to 3152 in b019cdc

    
           { 
        
               LOCK(m_most_recent_block_mutex); 
        
               a_recent_block = m_most_recent_block; 
        
           }

bitcoin/src/net_processing.cpp

Lines 3201 to 3206 in b019cdc

    
           { 
        
               LOCK(m_most_recent_block_mutex); 
        
               if (m_most_recent_block_hash == req.blockhash) 
        
                   recent_block = m_most_recent_block; 
        
               // Unlock m_most_recent_block_mutex to avoid cs_main lock inversion 
        
           }

bitcoin/src/net_processing.cpp

Lines 4763 to 4769 in b019cdc

    
           { 
        
               LOCK(m_most_recent_block_mutex); 
        
               if (m_most_recent_block_hash == pBestIndex->GetBlockHash()) { 
        
                   m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block)); 
        
                   fGotBlockFromCache = true; 
        
               } 
        
           }

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.

DrahtBot · 2022-01-14T02:24:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

hebasto · 2022-01-14T08:02:14Z

Concept ACK. Thanks!

hebasto

Approach ACK cac8366.

hebasto · 2022-01-14T08:22:12Z

src/net_processing.cpp

@@ -4704,7 +4704,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)

                    bool fGotBlockFromCache = false;
                    {
-                        LOCK(cs_most_recent_block);
+                        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.

theStack · 2022-01-25T15:16:35Z

Added another commit which reduces the scope of the lock most_recent_block_mutex, as suggested by @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.

theStack · 2022-05-01T23:58:04Z

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.

theStack · 2022-05-16T13:35:58Z

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.

furszy

Code ACK 83003ff with a small comment.

furszy · 2022-05-16T13:43:56Z

src/net_processing.cpp

+                    if (cached_cmpctblock_msg.has_value()) {
+                        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)); }

hebasto

ACK 83003ff

w0xlt

ACK 83003ff

maflcko · 2022-05-17T07:50:28Z

src/net_processing.cpp

                        }
                    }
-                    if (!fGotBlockFromCache) {
+                    if (cached_cmpctblock_msg.has_value()) {
+                        m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value()));


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.

… by propagating some negative capabilities 2b3373c refactor: Propagate negative `!m_tx_relay_mutex` capability (Hennadii Stepanov) 5a6e3c1 refactor: Propagate negative `!m_most_recent_block_mutex` capability (Hennadii Stepanov) Pull request description: This PR is a follow-up for bitcoin/bitcoin#22778 and bitcoin/bitcoin#24062, and it seems [required](bitcoin/bitcoin#24931 (comment)) for bitcoin/bitcoin#24931. See details in the commit messages. ACKs for top commit: ajtowns: ACK 2b3373c w0xlt: ACK bitcoin/bitcoin@2b3373c Tree-SHA512: 8a4bb9641af8d79824ec12e2d6bfce0e09524094b298a2edcdb2ab115fbaa21215b9c97a6b059f8aa023551071fd5798eca66ab8d262a3f97246a91d960850d0

…block_mutex` with Mutex 83003ff refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex (Sebastian Falbesoner) 8edd0d3 refactor: reduce scope of lock `m_most_recent_block_mutex` (Sebastian Falbesoner) Pull request description: This PR is related to bitcoin#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: https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L1650-L1655 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L1861-L1865 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L3149-L3152 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L3201-L3206 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L4763-L4769 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. ACKs for top commit: furszy: Code ACK 83003ff with a small comment. hebasto: ACK 83003ff w0xlt: ACK bitcoin@83003ff Tree-SHA512: 3df290cafd2f6c4d40afb9f14e822a77d9c1828e66f5e2233f3ac1deccc2b0a8290bc5fb8eb992f49d39e887b50bc0e9aad63e05db2d870791a8d409fb95695f

DrahtBot added P2P Refactoring labels Jan 14, 2022

hebasto mentioned this pull request Jan 14, 2022

Replace all of the RecursiveMutex instances with the Mutex ones #19303

Open

36 tasks

hebasto reviewed Jan 14, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 14, 2022

net processing: Only support version 2 compact blocks #20799

Merged

theStack force-pushed the 202201-refactor_replace_recursive_mutex_cs_last_block branch from cac8366 to 129043c Compare January 25, 2022 15:13

DrahtBot mentioned this pull request Mar 12, 2022

net processing: Move remaining globals into PeerManagerImpl #24543

Merged

DrahtBot added the Needs rebase label Apr 30, 2022

theStack changed the title ~~refactor: replace RecursiveMutex cs_most_recent_block with Mutex (and rename)~~ refactor: replace RecursiveMutex m_most_recent_block with Mutex May 1, 2022

theStack force-pushed the 202201-refactor_replace_recursive_mutex_cs_last_block branch from 129043c to 00b513f Compare May 1, 2022 23:52

theStack changed the title ~~refactor: replace RecursiveMutex m_most_recent_block with Mutex~~ refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex May 1, 2022

DrahtBot removed the Needs rebase label May 2, 2022

DrahtBot added the Needs rebase label May 16, 2022

theStack added 2 commits May 16, 2022 15:26

refactor: reduce scope of lock m_most_recent_block_mutex

8edd0d3

This avoids calling the non-trivial method `CConnman::PushMessage` within the critical section.

refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex

83003ff

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.

theStack force-pushed the 202201-refactor_replace_recursive_mutex_cs_last_block branch from 00b513f to 83003ff Compare May 16, 2022 13:27

furszy approved these changes May 16, 2022

View reviewed changes

DrahtBot removed the Needs rebase label May 16, 2022

hebasto approved these changes May 16, 2022

View reviewed changes

w0xlt approved these changes May 16, 2022

View reviewed changes

maflcko merged commit 0be1dc1 into bitcoin:master May 17, 2022

maflcko reviewed May 17, 2022

View reviewed changes

theStack deleted the 202201-refactor_replace_recursive_mutex_cs_last_block branch May 17, 2022 09:23

This was referenced May 20, 2022

refactor: Improve thread safety analysis by propagating some negative capabilities #25175

Merged

Strengthen thread safety assertions #24931

Merged

bitcoin locked and limited conversation to collaborators May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex #24062

refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex #24062

Uh oh!

theStack commented Jan 14, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Jan 14, 2022 •

edited

Loading

Uh oh!

hebasto commented Jan 14, 2022

Uh oh!

hebasto left a comment

Uh oh!

hebasto Jan 14, 2022 •

edited

Loading

Uh oh!

theStack Jan 14, 2022

Uh oh!

theStack commented Jan 25, 2022

Uh oh!

theStack commented May 1, 2022

Uh oh!

theStack commented May 16, 2022

Uh oh!

furszy left a comment

Uh oh!

furszy May 16, 2022

Uh oh!

hebasto left a comment

Uh oh!

w0xlt left a comment

Uh oh!

maflcko May 17, 2022

Uh oh!

hebasto May 17, 2022 •

edited

Loading

Uh oh!

Uh oh!

	{
	LOCK(m_most_recent_block_mutex);
	m_most_recent_block_hash = hashBlock;
	m_most_recent_block = pblock;
	m_most_recent_compact_block = pcmpctblock;
	}

	{
	LOCK(m_most_recent_block_mutex);
	a_recent_block = m_most_recent_block;
	a_recent_compact_block = m_most_recent_compact_block;
	}

	{
	LOCK(m_most_recent_block_mutex);
	if (m_most_recent_block_hash == req.blockhash)
	recent_block = m_most_recent_block;
	// Unlock m_most_recent_block_mutex to avoid cs_main lock inversion
	}

	{
	LOCK(m_most_recent_block_mutex);
	if (m_most_recent_block_hash == pBestIndex->GetBlockHash()) {
	m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block));
	fGotBlockFromCache = true;
	}
	}

		if (cached_cmpctblock_msg.has_value()) {
		m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value()));

refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex #24062

refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex #24062

Uh oh!

Conversation

theStack commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Jan 14, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

theStack commented Jan 25, 2022

Uh oh!

theStack commented May 1, 2022

Uh oh!

theStack commented May 16, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy May 16, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko May 17, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex #24062

refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex #24062

theStack commented Jan 14, 2022 •

edited

Loading

DrahtBot commented Jan 14, 2022 •

edited

Loading

hebasto Jan 14, 2022 •

edited

Loading

hebasto May 17, 2022 •

edited

Loading