validation: avoid potential deadlocks in ValidationInterface #15205

1. 
   jamesob commented at 7:08 pm on January 18, 2019: member

   Currently, the potential for a deadlock exists if ActivateBestChain() is called from within the ValidationInterface scheduler thread. This is because ABC itself avoids overrunning the VI queue by waiting for it to drain once it has passed a certain depth.

   To avoid this (or at least disallow it during CI runs), this change introduces thread_local state that gets set to indicate execution on ValidationInterface threads. If we’ve compiled with DEBUG_LOCKORDER, calling ABC from within a VI thread throws an exception. A small, necessary refactoring on CMainSignals is included.

   This changeset also includes a commit from #13168 that limits the platforms where we can rely on the use of thread_local.

2. disable thread_local on unreliable platforms

   See discussions here:
   
   - https://github.com/bitcoin/bitcoin/pull/11722#pullrequestreview-79322658
   - https://github.com/bitcoin/bitcoin/pull/13168#issuecomment-387181155

   315d84314a
3. jamesob force-pushed on Jan 18, 2019
4. in src/validationinterface.cpp:209 in 76d0fa3cd0 outdated

   204+{
   205+    m_internals->m_schedulerClient.AddToProcessQueue([func]() {
   206+        // Indicate that we're running in a ValidationInterface
   207+        // thread. This affects which functions can be called without risk of
   208+        // deadlock.
   209+        MarkRunningInQueue();
   

   ---

   ---

   

   ryanofsky commented at 7:22 pm on January 18, 2019:

   Wouldn’t it be simpler and more efficient to call MarkRunningInQueue once when the queue is created, instead of every time a notification added?

   ---

   

   jamesob commented at 7:29 pm on January 18, 2019:

   Good point! Will fix.

   ---

   

   jamesob commented at 7:46 pm on January 18, 2019:

   Fixed. Now executing MarkRunningInQueue once whenever the m_internals scheduler is reset. The change makes the refactoring strictly unneeded, but I think it’s still nice to have for testing and less duplication (though I’m happy to remove it too).
5. jamesob force-pushed on Jan 18, 2019
6. jamesob force-pushed on Jan 18, 2019
7. fanquake added the label Validation on Jan 18, 2019
8. 
   promag commented at 5:38 pm on January 19, 2019: member

   Restarted appveyor, unrelated error.
9. in src/validationinterface.h:186 in c024cc932a outdated

   175@@ -165,6 +176,11 @@ class CMainSignals {
   176     void MempoolEntryRemoved(CTransactionRef tx, MemPoolRemovalReason reason);
   177 
   178 public:
   179+    // Declare default *structors to avoid compilation issues due to the
   180+    // forward delcaration of MainSignalsInstance.
   

   ---

   ---

   

   practicalswift commented at 9:08 am on January 20, 2019:

   Should be “declaration” :-)
10. in src/validation.cpp:2682 in c024cc932a outdated

    2682+            throw std::logic_error(
    2683+                "This function cannot be called from within the ValidationInterface "
    2684+                "queue because it may result in a deadlock when attempting to "
    2685+                "drain the same queue below.");
    2686+        }
    2687+#endif
    

    ---

    ---

    

    jamesob commented at 1:37 pm on January 20, 2019:

    It occurs to me now that for futureproofing we should add similar code to SyncWithValidationInterfaceQueue(). Will add.
11. in src/validationinterface.h:68 in c024cc932a outdated

    62+ * Used to warn against calls to functions which may introduce deadlocks by
    63+ * interacting with the queue we're executing from.
    64+ *
    65+ * Only supported for platforms which HAVE_THREAD_LOCAL.
    66+ */
    67+bool IsRunningInValidationInterfaceQueue();
    

    ---

    ---

    

    practicalswift commented at 8:35 am on January 22, 2019:

    Should be inside #ifdef DEBUG_LOCKORDER ?
12. jamesob force-pushed on Jan 22, 2019
13. 
    jamesob commented at 2:33 pm on January 22, 2019: member

    Pushed an update futureproofing all calls to SyncWithValidationInterfaceQueue() (changes).
14. jamesob force-pushed on Jan 22, 2019
15. jamesob force-pushed on Jan 22, 2019
16. validation: avoid deadlocks due to ValidationInterface/ABC interaction

    Since ActivateBestChain will in certain cases block in order to allow the
    ValidationInterface queue to drain, it cannot safely be called from within
    ValidationInterface callbacks. This patch adds assertions preventing that when
    DEBUG_LOCKORDER.
    
    We slightly rework the ValidationInterface class, adding an
    `AddToProcessQueue()` method that handles scheduling a callback. When the
    scheduling queue is initiated, we make the thread aware it's managed by
    ValidationInterface.

    fa10bd4fea
17. test: ensure we disallow calls to ABC within ValidationInterface

    Also adds default constructor and destructor to CMainSignals to avoid build
    errors due to the forward declaration of MainSignalsInstance.

    93e09eeced
18. jamesob force-pushed on Jan 22, 2019
19. 
    jamesob commented at 4:14 pm on January 22, 2019: member

    I’ve taken @practicalswift’s advice on declaration conditional on #ifdef DEBUG_LOCKORDER but I kinda hate how many ifdefs that’s introduced.
20. 
    jamesob commented at 8:44 pm on January 22, 2019: member

    We discussed a better approach today on IRC (http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-22.html), which is to conditionally move all Schedule tasks to blocking execution (instead of executing them on a separate thread) in the presence of some flag - that way we’ll be able to make use of all the existing deadlock detection utilities when, say, running CI tests.
21. jamesob closed this on Jan 22, 2019

    ---

22. MarcoFalke locked this on Dec 16, 2021

Contributors
jamesob ryanofsky promag practicalswift

Labels
Validation