From 51630d2e5ebaa533ed3035761e18c287e30bfa32 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 30 Aug 2021 10:07:28 +0200 Subject: [PATCH] Merge bitcoin/bitcoin#22824: refactor: remove RecursiveMutex cs_nBlockSequenceId 0bd882b7405414b5355e69a9fdcd7a533e504b6b 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 #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. ACKs for top commit: Zero-1729: ACK 0bd882b promag: Code review ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b. hebasto: ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b Tree-SHA512: 435271ac8f877074099ddb31436665b500e555f7cab899e5c8414af299b154d1249996be500e8fdeff64e4639bcaf7386e12510b738ec6f20e415e7e35afaea9 --- src/validation.cpp | 5 +---- src/validation.h | 5 ++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 3387f746b9773..9a7336915fda7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3688,10 +3688,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())) { if (!(pindex->nStatus & BLOCK_CONFLICT_CHAINLOCK)) { setBlockIndexCandidates.insert(pindex); diff --git a/src/validation.h b/src/validation.h index 3bfe6b872d1e6..1b6b1bb28c362 100644 --- a/src/validation.h +++ b/src/validation.h @@ -629,9 +629,8 @@ class CChainState * 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. */ @@ -828,7 +827,7 @@ class CChainState 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;