cover DisconnectBlock with lock annotation #23661
Conversation
CoinsTip() access requires cs_main and therefore so should this function.
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Concept ACK on preventatively ensuring the mutex continues to be held over future changes when calling this function.
| * When FAILED is returned, view is left in an indeterminate state. */ | ||
| DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view) | ||
| { | ||
| AssertLockHeld(::cs_main); |
There was a problem hiding this comment.
This assertion in the definition seems unneeded? IIUC the lock annotation in the function declaration enforces it.
There was a problem hiding this comment.
Never mind, just saw the developer notes recommend this ("Combine annotations in function declarations with
run-time asserts in function definitions").
| // Block (dis)connection on a given view: | ||
| DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view); | ||
| DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view) | ||
| EXCLUSIVE_LOCKS_REQUIRED(::cs_main); |
There was a problem hiding this comment.
Why use the ::cs_main global reference here rather than cs_main?
There was a problem hiding this comment.
I think for new code we prefer ::global, unless the name of the global is prefixed with g_..., in which case ::g_... and g_... are allowed.
|
ACK 7da4a8f |
d3c01ad cover DisconnectBlock with lock annotation (James O'Beirne) Pull request description: While reviewing #23630, I noticed that `DisconnectBlock` is uncovered by lock annotations. CoinsTip() access requires cs_main and therefore so should this function. ACKs for top commit: jonatack: ACK d3c01ad Tree-SHA512: 3e2b0247c138b31deeadcd48eb3f7bc8d32c0b6bb6d6e94ccf8ea0cbbc50b1b35d83f662eee432f2bd2d87a3fe9c94604da806ec711df93298bfb0ab34a5a05b
Summary: CoinsTip() access requires cs_main and therefore so should this function. This is a backport of [[bitcoin/bitcoin#23661 | core#23661]] Test Plan: With clang + DEBUG `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12456
3 participants
While reviewing #23630, I noticed that
DisconnectBlockis uncovered by lock annotations. CoinsTip() access requires cs_main and therefore so should this function.