refactor: remove RecursiveMutex cs_nBlockSequenceId by theStack · Pull Request #22824 · bitcoin/bitcoin

theStack · 2021-08-28T10:15:37Z

The RecursiveMutex cs_nBlockSequenceId is only used at one place in CChainState::ReceivedBlockTransactions() to atomically read-and-increment the nBlockSequenceId member:

bitcoin/src/validation.cpp

Lines 2973 to 2976 in 83daf47

    
           { 
        
               LOCK(cs_nBlockSequenceId); 
        
               pindex->nSequenceId = nBlockSequenceId++; 
        
           }

~~For this simple use-case, we can make the member std::atomic instead to achieve the same result (see https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith).~~

This is related to #19303. As suggested in the issue, I first planned to change the RecursiveMutex to Mutex (still possible if the change doesn't get Concept ACKs), but using a Mutex for this simple operation seems to be overkill. Note that at the time when this mutex was introduced (PR #3370, commit 75f51f2) std::atomic were not used in the codebase yet -- according to git log -S std::atomic they have first appeared in 2016 (commit 7e908c7), probably also because the compilers didn't support them properly earlier.

At this point, the cs_main lock is set, hence we can use a plain int for the member and mark it as guarded by cs_main.

hebasto · 2021-08-28T10:20:26Z

Concept ACK.

Zero-1729 · 2021-08-28T11:47:41Z

Concept ACK

ryanofsky

Code review ACK 98d60b1c54112c772663e3add6665a65d264498c. Funny, I suggested this exact change yesterday #22564 (comment)!

hebasto

ACK 98d60b1c54112c772663e3add6665a65d264498c. I have reviewed the code and it looks OK, I agree it can be merged.

Thanks for slaying another RecursiveMutex ⚔️

maflcko · 2021-08-28T14:11:41Z

As this is only accessed in one place (that already has cs_main), it would also be possible to make it a plain int

hebasto · 2021-08-28T14:16:04Z

As this is only accessed in one place (that already has cs_main), it would also be possible to make it a plain int

Doesn't it make code fragile for future changes?

Zero-1729

crACK 98d60b1 🧉

DrahtBot · 2021-08-29T04:01:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22564 ([WIP] refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)

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.

maflcko · 2021-08-29T07:33:59Z

Doesn't it make code fragile for future changes?

How?

hebasto · 2021-08-29T07:37:26Z

Doesn't it make code fragile for future changes?

How?

Another access to the variable will be added in a new place, but the variable remains plain int32_t.

maflcko · 2021-08-29T08:08:54Z

We are using lock annotations to protect against this, no?

hebasto · 2021-08-29T08:13:17Z

We are using lock annotations to protect against this, no?

Thread safety annotations go with a mutex. Without a mutex (this PR) nothing will guard the access to a plain int32_t, no?

maflcko · 2021-08-29T09:10:56Z

Yes, I mentioned the validation mutex cs_main, which seems fine because the block's sequence is only used by validation.

hebasto · 2021-08-29T11:00:57Z

@MarcoFalke

Yes, I mentioned the validation mutex cs_main, which seems fine because the block's sequence is only used by validation.

You are right. The following change:

diff --git a/src/validation.cpp b/src/validation.cpp
index ec457da5c..9944e3155 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2970,10 +2970,7 @@ void CChainState::ReceivedBlockTransactions(const CBlock& block, CBlockIndex* pi
             CBlockIndex *pindex = queue.front();
             queue.pop_front();
             pindex->nChainTx = (pindex->pprev ? pindex->pprev->nChainTx : 0) + pindex->nTx;
-            {
-                LOCK(cs_nBlockSequenceId);
-                pindex->nSequenceId = nBlockSequenceId++;
-            }
+            pindex->nSequenceId = nBlockSequenceId++;
             if (m_chain.Tip() == nullptr || !setBlockIndexCandidates.value_comp()(pindex, m_chain.Tip())) {
                 setBlockIndexCandidates.insert(pindex);
             }
diff --git a/src/validation.h b/src/validation.h
index b80fa9d32..58eed48c9 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -558,9 +558,8 @@ protected:
      * Every received block is assigned a unique and increasing identifier, so we
      * know which one to give priority in case of a fork.
      */
-    RecursiveMutex cs_nBlockSequenceId;
     /** Blocks loaded from disk are assigned id 0, so start the counter at 1. */
-    int32_t nBlockSequenceId = 1;
+    int32_t nBlockSequenceId GUARDED_BY(::cs_main) = 1;
     /** Decreasing counter (used by subsequent preciousblock calls). */
     int32_t nBlockReverseSequenceId = -1;
     /** chainwork for the last block that preciousblock has been applied to. */
@@ -749,7 +748,7 @@ public:
 
     void PruneBlockIndexCandidates();
 
-    void UnloadBlockIndex();
+    void UnloadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /** Check whether we are doing an initial block download (synchronizing from disk or network) */
     bool IsInitialBlockDownload() const;

looks more preferable.

The RecursiveMutex cs_nBlockSequenceId is only used at one place in CChainState::ReceivedBlockTransactions() to atomically read-and-increment the nBlockSequenceId member. At this point, the cs_main lock is set, hence we can use a plain int for the member and mark it as guarded by cs_main.

theStack · 2021-08-29T11:38:21Z

@MarcoFalke @hebasto: Thanks, that makes sense. I agree, considering that CChainState::ReceivedBlockTransactions() holds cs_main (which I wasn't aware of before), just using a plain int and marking it as guarded by cs_main is preferred.

Updated PR title and description, force-pushed with the suggested change.

Zero-1729

ACK 0bd882b

promag

Code review ACK 0bd882b.

hebasto

ACK 0bd882b

jonatack · 2021-08-30T09:28:00Z

Markdown <strike></strike> markup doesn't translate well to text formatting in the merge commit description. Seems good not to rely on markdown in the PR description for it to make sense, especially strike-through formatting.

0bd882b refactor: remove RecursiveMutex cs_nBlockSequenceId (Sebastian Falbesoner) Pull request description: The RecursiveMutex `cs_nBlockSequenceId` is only used at one place in `CChainState::ReceivedBlockTransactions()` to atomically read-and-increment the nBlockSequenceId member: https://github.com/bitcoin/bitcoin/blob/83daf47898f8a79cb20d20316c64becd564cf54c/src/validation.cpp#L2973-L2976 ~~For this simple use-case, we can make the member `std::atomic` instead to achieve the same result (see https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith).~~ ~~This is related to bitcoin#19303. As suggested in the issue, I first planned to change the `RecursiveMutex` to `Mutex` (still possible if the change doesn't get Concept ACKs), but using a Mutex for this simple operation seems to be overkill. Note that at the time when this mutex was introduced (PR bitcoin#3370, commit 75f51f2) `std::atomic` were not used in the codebase yet -- according to `git log -S std::atomic` they have first appeared in 2016 (commit 7e908c7), probably also because the compilers didn't support them properly earlier.~~ At this point, the cs_main lock is set, hence we can use a plain int for the member and mark it as guarded by cs_main. ACKs for top commit: Zero-1729: ACK 0bd882b promag: Code review ACK 0bd882b. hebasto: ACK 0bd882b Tree-SHA512: 435271ac8f877074099ddb31436665b500e555f7cab899e5c8414af299b154d1249996be500e8fdeff64e4639bcaf7386e12510b738ec6f20e415e7e35afaea9

0bd882b refactor: remove RecursiveMutex cs_nBlockSequenceId (Sebastian Falbesoner) Pull request description: The RecursiveMutex `cs_nBlockSequenceId` is only used at one place in `CChainState::ReceivedBlockTransactions()` to atomically read-and-increment the nBlockSequenceId member: https://github.com/bitcoin/bitcoin/blob/83daf47898f8a79cb20d20316c64becd564cf54c/src/validation.cpp#L2973-L2976 ~~For this simple use-case, we can make the member `std::atomic` instead to achieve the same result (see https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith).~~ ~~This is related to bitcoin#19303. As suggested in the issue, I first planned to change the `RecursiveMutex` to `Mutex` (still possible if the change doesn't get Concept ACKs), but using a Mutex for this simple operation seems to be overkill. Note that at the time when this mutex was introduced (PR dashpay#3370, commit 75f51f2) `std::atomic` were not used in the codebase yet -- according to `git log -S std::atomic` they have first appeared in 2016 (commit 7e908c7), probably also because the compilers didn't support them properly earlier.~~ At this point, the cs_main lock is set, hence we can use a plain int for the member and mark it as guarded by cs_main. ACKs for top commit: Zero-1729: ACK 0bd882b promag: Code review ACK 0bd882b. hebasto: ACK 0bd882b Tree-SHA512: 435271ac8f877074099ddb31436665b500e555f7cab899e5c8414af299b154d1249996be500e8fdeff64e4639bcaf7386e12510b738ec6f20e415e7e35afaea9

fanquake added the Validation label Aug 28, 2021

hebasto mentioned this pull request Aug 28, 2021

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

Open

35 tasks

ryanofsky approved these changes Aug 28, 2021

View reviewed changes

hebasto approved these changes Aug 28, 2021

View reviewed changes

Zero-1729 approved these changes Aug 28, 2021

View reviewed changes

DrahtBot mentioned this pull request Aug 29, 2021

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

theStack changed the title ~~refactor: remove RecursiveMutex cs_nBlockSequenceId, use std::atomic instead~~ refactor: remove RecursiveMutex cs_nBlockSequenceId Aug 29, 2021

theStack force-pushed the 202108-refactor-use_atomic_for_nblocksequenceid branch from 98d60b1 to 0bd882b Compare August 29, 2021 11:33

Zero-1729 approved these changes Aug 29, 2021

View reviewed changes

promag reviewed Aug 29, 2021

View reviewed changes

bitcoin deleted a comment from kolodii4 Aug 29, 2021

hebasto approved these changes Aug 29, 2021

View reviewed changes

maflcko merged commit 7be143a into bitcoin:master Aug 30, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 30, 2022

	{
	LOCK(cs_nBlockSequenceId);
	pindex->nSequenceId = nBlockSequenceId++;
	}

Conversation

theStack commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Aug 28, 2021

Uh oh!

Zero-1729 commented Aug 28, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • 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!

maflcko commented Aug 28, 2021

Uh oh!

hebasto commented Aug 28, 2021

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 29, 2021

Conflicts

Uh oh!

maflcko commented Aug 29, 2021

Uh oh!

hebasto commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 29, 2021

Uh oh!

hebasto commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 29, 2021

Uh oh!

hebasto commented Aug 29, 2021

Uh oh!

theStack commented Aug 29, 2021

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

theStack commented Aug 28, 2021 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto commented Aug 29, 2021 •

edited

Loading

hebasto commented Aug 29, 2021 •

edited

Loading

jonatack commented Aug 30, 2021 •

edited

Loading