error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed #28146

Is there an existing issue for this?

• I have searched the existing issues

Current behaviour

check failed

Expected behaviour

not fail

Steps to reproduce

?

Relevant log output

https://cirrus-ci.com/task/4769037708689408?logs=ci#L3768

test/validationinterface_tests.cpp(93): error: in "validationinterface_tests/unregister_all_during_call": check destroyed has failed

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

current master

Operating system and version

arm CI

Machine specifications

No response

TestInterface::Call(); should be blocking on the destructor, which sets destroyed, so I have no idea how this could happen.

I was able to reproduce this by adding some sleeps:

<details> <summary>diff on bc4f6b13feb29146b7e10e86f93dc7f6fb6937f2 </summary> <br>

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index d344c8bfb..bb645ae94 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -17,6 +17,9 @@
 #include <unordered_map>
 #include <utility>
 
+#include <chrono>
+#include <thread>
+
 std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;
 
 /**
@@ -86,7 +89,9 @@ public:
             {
                 REVERSE_LOCK(lock);
                 f(*it->callbacks);
+                std::this_thread::sleep_for(1ms);
             }
+            LogPrintf("it->count %v\n", it->count);
             it = --it->count ? std::next(it) : m_list.erase(it);
         }
     }
@@ -182,6 +187,7 @@ void SyncWithValidationInterfaceQueue()
         auto local_name = (name);                              \
         LOG_EVENT("Enqueuing " fmt, local_name, __VA_ARGS__);  \
         m_internals->m_schedulerClient.AddToProcessQueue([=] { \
+            std::this_thread::sleep_for(800us);                  \
             LOG_EVENT(fmt, local_name, __VA_ARGS__);           \
             event();                                           \
         });                                                    \
@@ -194,7 +200,6 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
     // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which
     // the chain actually updates. One way to ensure this is for the caller to invoke this signal
     // in the same critical section where the chain is updated
-
     auto event = [pindexNew, pindexFork, fInitialDownload, this] {
         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); });
     };

</details>

In the CI logs, UpdatedBlockTip and BlockChecked run at ~ the same time. I think what happens is:

• BlockChecked calls Iterate which increments it->count (count = 2) with m_mutex
• m_on_call gets called without m_mutex
• In the scheduler thread, Iterate is called and increments it->count (count = 3) with m_mutex. (UpdatedBlockTip runs in the scheduler thread). Then m_mutex is released to call UpdatedBlockTip
• m_on_call calls UnregisterAllValidationInterfaces which will decrement it->count (count = 2) with m_mutex.
• m_on_call calls UnregisterAllValidationInterfaces which is a no-op.
• The BlockChecked callback finishes and since the count = 2, the element isn't erased so the destructor isn't called. This fails the test

UpdatedBlockTip runs in the scheduler thread

Thanks. That sounds like a bug. The unit test should be single threaded, or alternatively drop all async notifications before starting.

Let's continue discussion in https://github.com/bitcoin/bitcoin/pull/28150