Inconsistent locking behavior in LoadChainTip #15967

LoadChainTip requires cs_main to be locked. Under some conditions, it may call ActivateBestChain. However, ActivateBestChain requires cs_main NOT to be taken when called.

Confirming that there appears to be an inconsistency:

$ grep -B11 '^        if (!ActivateBestChain(state, chainparams)) {' src/validation.cpp
bool LoadChainTip(const CChainParams& chainparams)
{
    AssertLockHeld(cs_main);

    if (chainActive.Tip() && chainActive.Tip()->GetBlockHash() == pcoinsTip->GetBestBlock()) return true;

    if (pcoinsTip->GetBestBlock().IsNull() && mapBlockIndex.size() == 1) {
        // In case we just added the genesis block, connect it now, so
        // that we always have a chainActive.Tip() when we return.
        LogPrintf("%s: Connecting genesis block...\n", __func__);
        CValidationState state;
        if (!ActivateBestChain(state, chainparams)) {
$ grep -A2 -E '^bool ActivateBestChain' src/validation.cpp
bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock) {
    return g_chainstate.ActivateBestChain(state, chainparams, std::move(pblock));
}
$ grep -A5 -E '^bool CChainState::ActivateBestChain\(' src/validation.cpp
bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock) {
    // Note that while we're often called here from ProcessNewBlock, this is
    // far from a guarantee. Things in the P2P/RPC will often end up calling
    // us in the middle of ProcessNewBlock - do not assume pblock is set
    // sanely for performance or correctness!
    AssertLockNotHeld(cs_main);
$ git diff
diff --git a/src/validation.h b/src/validation.h
index 7ab6adaf3..5f19f32b4 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -257,7 +257,7 @@ bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::P
  * May not be called with cs_main held. May not be called in a
  * validationinterface callback.
  */
-bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock = std::shared_ptr<const CBlock>());
+bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock = std::shared_ptr<const CBlock>()) LOCKS_EXCLUDED(cs_main);
 CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);

 /** Guess verification progress (as a fraction between 0.0=genesis and 1.0=current tip). */

$ make
…
 validation.cpp:3959:14: error: cannot call function 'ActivateBestChain' while mutex 'cs_main' is held [-Werror,-Wthread-safety-analysis]
        if (!ActivateBestChain(state, chainparams)) {
             ^
1 error generated.

I just submitted #15971 to add compile-time checking for some negative locking requirements in the validation code, but that PR does not address this case.

Updated: Had totally forgotten that I submitted #15721 -- which adds LOCKS_EXCLUDED(cs_main) to CChainState::ActivateBestChain -- back in April too :-)