Merge #18742: miner: Avoid stack-use-after-return in validationinterface

7777f2a4bb1f9d843bc50a4e35085cfbb2808780 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
fa5ceb25fce2200edf6b8ebfa6d4f01ed6774b95 test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue (MarcoFalke)
fa770ce7fe67685c43780e219d8232efbee0bb8e validationinterface: Rework documentation, Rename pwalletIn to callbacks (MarcoFalke)
fab6d060ce5f580db538070beec1c5518c8c777c test: Add unregister_validation_interface_race test (MarcoFalke)

Pull request description:

  When a validationinterface has itself unregistered in one thread, but is about to get executed in another thread [1], there is a race:

  * The validationinterface destructing itself
  * The validationinterface getting dereferenced for execution

  [1] https://github.com/bitcoin/bitcoin/blob/64139803f1225dab26197a20314109d37fa87d5f/src/validationinterface.cpp#L82-L83

  This happens in the miner. More generally it happens everywhere where at least one thread is generating notifications and another one is unregistering a validationinterface.

  This issue has been fixed in commit ab31b9d6fe7b39713682e3f52d11238dbe042c16, but the fix has not been applied to the miner.

  Example where this happened in practice: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675322230#L4414

ACKs for top commit:
  promag:
    Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780.
  laanwj:
    Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780

Tree-SHA512: 8087119243c71ba18a823a63515f3730d127162625d8729024278b447af29e2ff206f4840ee3d90bf84f93a2c5ab73b76c7e7044c83aa93b5b51047a166ec3d3

1 files changed, 22 insertions, 13 deletions

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index 11000774c..9437f9c81 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -89,22 +89,26 @@ public:
 
 static CMainSignals g_signals;
 
-void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) {
+void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler)
+{
     assert(!m_internals);
     m_internals.reset(new MainSignalsInstance(&scheduler));
 }
 
-void CMainSignals::UnregisterBackgroundSignalScheduler() {
+void CMainSignals::UnregisterBackgroundSignalScheduler()
+{
     m_internals.reset(nullptr);
 }
 
-void CMainSignals::FlushBackgroundCallbacks() {
+void CMainSignals::FlushBackgroundCallbacks()
+{
     if (m_internals) {
         m_internals->m_schedulerClient.EmptyQueue();
     }
 }
 
-size_t CMainSignals::CallbacksPending() {
+size_t CMainSignals::CallbacksPending()
+{
     if (!m_internals) return 0;
     return m_internals->m_schedulerClient.CallbacksPending();
 }
@@ -114,10 +118,11 @@ CMainSignals& GetMainSignals()
     return g_signals;
 }
 
-void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn) {
-    // Each connection captures pwalletIn to ensure that each callback is
-    // executed before pwalletIn is destroyed. For more details see #18338.
-    g_signals.m_internals->Register(std::move(pwalletIn));
+void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> callbacks)
+{
+    // Each connection captures the shared_ptr to ensure that each callback is
+    // executed before the subscriber is destroyed. For more details see #18338.
+    g_signals.m_internals->Register(std::move(callbacks));
 }
 
 void RegisterValidationInterface(CValidationInterface* callbacks)
@@ -132,24 +137,28 @@ void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> c
     UnregisterValidationInterface(callbacks.get());
 }
 
-void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
+void UnregisterValidationInterface(CValidationInterface* callbacks)
+{
     if (g_signals.m_internals) {
-        g_signals.m_internals->Unregister(pwalletIn);
+        g_signals.m_internals->Unregister(callbacks);
     }
 }
 
-void UnregisterAllValidationInterfaces() {
+void UnregisterAllValidationInterfaces()
+{
     if (!g_signals.m_internals) {
         return;
     }
     g_signals.m_internals->Clear();
 }
 
-void CallFunctionInValidationInterfaceQueue(std::function<void ()> func) {
+void CallFunctionInValidationInterfaceQueue(std::function<void()> func)
+{
     g_signals.m_internals->m_schedulerClient.AddToProcessQueue(std::move(func));
 }
 
-void SyncWithValidationInterfaceQueue() {
+void SyncWithValidationInterfaceQueue()
+{
     AssertLockNotHeld(cs_main);
     // Block until the validation queue drains
     std::promise<void> promise;