Document validationinterace callback blocking deadlock potential. #13402

1. 
   TheBlueMatt commented at 8:42 pm on June 5, 2018: member

   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.

2. Document validationinterace callback blocking deadlock potential. 25bc9615b7
3. fanquake added the label Docs on Jun 5, 2018
4. randolf approved
5. in src/validation.cpp:2708 in 25bc9615b7

   2703@@ -2704,6 +2704,9 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
   2704             // Block until the validation queue drains. This should largely
   2705             // never happen in normal operation, however may happen during
   2706             // reindex, causing memory blowup if we run too far ahead.
   2707+            // Note that if a validationinterface callback ends up calling
   2708+            // ActivateBestChain this may lead to a deadlock! We should
   

   ---

   ---

   

   ryanofsky commented at 2:01 pm on June 6, 2018:

   Maybe add that this could lead to a deadlock “because ActiveBestChain won’t be able to acquire m_cs_chainstate”, if this is correct, to avoid making a puzzle of where the deadlock is.
6. 
   ryanofsky commented at 2:05 pm on June 6, 2018: member

   utACK 25bc9615b7480e4ba2c482a6f0e7e3b33f50e6e0 (this is a trivial change that only adds comments)
7. 
   DrahtBot commented at 3:22 pm on June 7, 2018: member

   • #13413 ([net,mempool] Call AcceptToMemoryPool() asynchronously in p2p by skeees)
   • #13399 (rpc: Add submitblockheader by MarcoFalke)
   • #13168 (Thread names in logs and deadlock debug tools (take 2) by jamesob)
   • #13144 (RPC: Improve error messages on RPC endpoints that use GetTransaction by jimpo)
   • #13083 (Add compile time checking for cs_main runtime locking assertions by practicalswift)
   • #12934 ([net] [validation] Call ProcessNewBlock() asynchronously by skeees)
   • #12407 (Ensure nStatus is set properly for all invalid blocks by jamesob)

   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.

8. in src/validation.h:249 in 25bc9615b7

   245@@ -245,7 +246,8 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
   246 /**
   247  * Process incoming block headers.
   248  *
   249- * Call without cs_main held.
   250+ * May not be called with cs_main held. May not be called in a
   

   ---

   ---

   

   sipa commented at 0:27 am on June 15, 2018:

   Can you add an AssertLockNotHeld(cs_main) to ProcessNewBlockHeaders then?
9. in src/validation.h:457 in 25bc9615b7

   451@@ -445,7 +452,11 @@ inline CBlockIndex* LookupBlockIndex(const uint256& hash)
   452 /** Find the last common block between the parameter chain and a locator. */
   453 CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator);
   454 
   455-/** Mark a block as precious and reorganize. */
   456+/** Mark a block as precious and reorganize.
   457+ *
   458+ * May not be called with cs_main held. May not be called in a
   

   ---

   ---

   

   sipa commented at 0:27 am on June 15, 2018:

   Same here for PreciousBlock.
10. 
    sipa commented at 0:28 am on June 15, 2018: member

    utACK 25bc9615b7480e4ba2c482a6f0e7e3b33f50e6e0, just nits.
11. 
    MarcoFalke commented at 1:58 pm on June 15, 2018: member

    utACK 25bc9615b7480e4ba2c482a6f0e7e3b33f50e6e0
12. MarcoFalke merged this on Jun 15, 2018
13. MarcoFalke closed this on Jun 15, 2018

    ---

14. MarcoFalke referenced this in commit 43fa3554b7 on Jun 15, 2018
15. laanwj referenced this in commit b641f60425 on Jul 9, 2018
16. jasonbcox referenced this in commit 4f5833a91e on Sep 27, 2019
17. PastaPastaPasta referenced this in commit ae16904b51 on Jul 7, 2020
18. PastaPastaPasta referenced this in commit db330eedae on Jul 9, 2020
19. PastaPastaPasta referenced this in commit 19484930b9 on Jul 9, 2020
20. MarcoFalke locked this on Sep 8, 2021

Contributors
TheBlueMatt randolf ryanofsky DrahtBot sipa MarcoFalke

Labels
Docs