Merge #13402: Document validationinterace callback blocking deadlock potential.

25bc9615b7 Document validationinterace callback blocking deadlock potential. (Matt Corallo)

Pull request description:

  From the branches-I've-had-lying-around-and-forgot-to-PR department...

  This is a comment-only PR, but the comments point out an API quirk that isn't exactly trivial. None of our use-cases right now hit this, but if we were to call SyncWithValidationInterfaceQueue (eg to limit queue depth) in ATMP, I'm pretty sure we'd hit a deadlock there.

Tree-SHA512: 889dd8fc9eb15d1f2aa5ca467e783bc8f07bc543b166b032741795b0db7a0df11a2846d3cb7c69bafa8d1acf970021001b742f52be06725a932813230c5b4a7b

2 files changed, 18 insertions, 4 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index bbf2389d34..d94b7c7d06 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2698,6 +2698,9 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
             // Block until the validation queue drains. This should largely
             // never happen in normal operation, however may happen during
             // reindex, causing memory blowup if we run too far ahead.
+            // Note that if a validationinterface callback ends up calling
+            // ActivateBestChain this may lead to a deadlock! We should
+            // probably have a DEBUG_LOCKORDER test for this in the future.
             SyncWithValidationInterfaceQueue();
         }
 
diff --git a/src/validation.h b/src/validation.h
index 04f5b6cb80..b9c2f6c023 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -233,7 +233,8 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
  * Note that we guarantee that either the proof-of-work is valid on pblock, or
  * (and possibly also) BlockChecked will have been called.
  * 
- * Call without cs_main held.
+ * May not be called with cs_main held. May not be called in a
+ * validationinterface callback.
  *
  * @param[in]   pblock  The block we want to process.
  * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers.
@@ -245,7 +246,8 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
 /**
  * Process incoming block headers.
  *
- * Call without cs_main held.
+ * May not be called with cs_main held. May not be called in a
+ * validationinterface callback.
  *
  * @param[in]  block The block headers themselves
  * @param[out] state This may be set to an Error state if any error occurred processing them
@@ -278,7 +280,12 @@ void ThreadScriptCheck();
 bool IsInitialBlockDownload();
 /** Retrieve a transaction (from memory pool, or from disk, if possible) */
 bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, bool fAllowSlow = false, CBlockIndex* blockIndex = nullptr);
-/** Find the best known block, and make it the tip of the block chain */
+/**
+ * Find the best known block, and make it the tip of the block chain
+ *
+ * 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>());
 CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
 
@@ -445,7 +452,11 @@ inline CBlockIndex* LookupBlockIndex(const uint256& hash)
 /** Find the last common block between the parameter chain and a locator. */
 CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator);
 
-/** Mark a block as precious and reorganize. */
+/** Mark a block as precious and reorganize.
+ *
+ * May not be called with cs_main held. May not be called in a
+ * validationinterface callback.
+ */
 bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIndex *pindex);
 
 /** Mark a block as invalid. */