-
Notifications
You must be signed in to change notification settings - Fork 35.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: remove RecursiveMutex cs_nBlockSequenceId #22824
refactor: remove RecursiveMutex cs_nBlockSequenceId #22824
Conversation
Concept ACK. |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 98d60b1. Funny, I suggested this exact change yesterday #22564 (comment)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 98d60b1. I have reviewed the code and it looks OK, I agree it can be merged.
Thanks for slaying another RecursiveMutex
⚔️
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK 98d60b1 🧉
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
How? |
Another access to the variable will be added in a new place, but the variable remains plain |
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 |
Yes, I mentioned the validation mutex |
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.
98d60b1
to
0bd882b
Compare
@MarcoFalke @hebasto: Thanks, that makes sense. I agree, considering that Updated PR title and description, force-pushed with the suggested change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 0bd882b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 0bd882b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 0bd882b
Markdown |
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
The RecursiveMutex
cs_nBlockSequenceId
is only used at one place inCChainState::ReceivedBlockTransactions()
to atomically read-and-increment the nBlockSequenceId member:bitcoin/src/validation.cpp
Lines 2973 to 2976 in 83daf47
For this simple use-case, we can make the memberstd::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 theRecursiveMutex
toMutex
(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 togit 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.