cover DisconnectBlock with lock annotation #23661

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:2021-12-disconnect-annotation changing 2 files +3 −1

jamesob commented at 3:38 PM on December 3, 2021: member

While reviewing #23630, I noticed that DisconnectBlock is uncovered by lock annotations. CoinsTip() access requires cs_main and therefore so should this function.

cover DisconnectBlock with lock annotation

CoinsTip() access requires cs_main and therefore so should this function.

7da4a8ffb3

MarcoFalke added the label Refactoring on Dec 3, 2021
in src/validation.cpp:1488 in 7da4a8ffb3
```
1484 | @@ -1485,6 +1485,7 @@ int ApplyTxInUndo(Coin&& undo, CCoinsViewCache& view, const COutPoint& out)
1485 |   *  When FAILED is returned, view is left in an indeterminate state. */
1486 |  DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
1487 |  {
1488 | +    AssertLockHeld(::cs_main);
```
jonatack commented at 12:25 PM on December 6, 2021:

This assertion in the definition seems unneeded? IIUC the lock annotation in the function declaration enforces it.

jonatack commented at 12:36 PM on December 6, 2021:

Never mind, just saw the developer notes recommend this ("Combine annotations in function declarations with run-time asserts in function definitions").

in src/validation.h:722 in 7da4a8ffb3

 717 | @@ -718,7 +718,8 @@ class CChainState
 718 |      bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 719 |  
 720 |      // Block (dis)connection on a given view:
 721 | -    DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view);
 722 | +    DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
 723 | +        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

jonatack commented at 12:28 PM on December 6, 2021:

Why use the ::cs_main global reference here rather than cs_main?

MarcoFalke commented at 12:35 PM on December 6, 2021:

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.

jonatack commented at 12:37 PM on December 6, 2021:

Thanks.

jonatack commented at 12:30 PM on December 6, 2021: member

Concept ACK on preventatively ensuring the mutex continues to be held over future changes when calling this function.
jonatack commented at 12:37 PM on December 6, 2021: member

ACK 7da4a8ffb3c9807c59f8ba63781169e4045594ba
MarcoFalke merged this on Dec 6, 2021
MarcoFalke closed this on Dec 6, 2021
sidhujag referenced this in commit a2ed5db71b on Dec 7, 2021
RandyMcMillan referenced this in commit 2768158bdd on Dec 23, 2021
Fabcien referenced this in commit c2a48fe20b on Nov 10, 2022
DrahtBot locked this on Dec 6, 2022

Contributors

Labels

Linked (view graph)

#23630 Remove GetSpendHeight