refactor: remove RecursiveMutex cs_nBlockSequenceId #22824

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 #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 75f51f2a63e0ebe34ab290c2b7141dd240b98c3b) std::atomic were not used in the codebase yet -- according to git log -S std::atomic they have first appeared in 2016 (commit 7e908c7b826cedbf29560ce7a668af809ee71524), 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.

Concept ACK.

Concept ACK

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

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

Thanks for slaying another RecursiveMutex :crossed_swords:

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

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?

crACK 98d60b1 🧉

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Doesn't it make code fragile for future changes?

How?

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.

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

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?

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

@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.

@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.

ACK 0bd882b

Code review ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b.

ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b

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.