Skip to content

Commit

Permalink
Merge bitcoin#22824: refactor: remove RecursiveMutex cs_nBlockSequenceId
Browse files Browse the repository at this point in the history
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
  • Loading branch information
MarcoFalke committed Aug 30, 2021
2 parents 83daf47 + 0bd882b commit 7be143a
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 7 deletions.
5 changes: 1 addition & 4 deletions src/validation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
5 changes: 2 additions & 3 deletions src/validation.h
Original file line number Diff line number Diff line change
Expand Up @@ -558,9 +558,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. */
Expand Down Expand Up @@ -749,7 +748,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;
Expand Down

0 comments on commit 7be143a

Please sign in to comment.