Remove RegisterValidationInterface() #26210

1. 
   vasild commented at 8:40 am on September 30, 2022: contributor

   RegisterValidationInterface() takes a bare pointer, remembers it and other threads use it afterwards. This puts a burden on the caller to properly maintain the lifetime of the object which adds complexity and is a source of bugs (https://github.com/bitcoin/bitcoin/issues/25365, #26188 (review)).

   shared_ptr exists with the purpose of resolving that problem exactly. We already have RegisterSharedValidationInterface(). I think it should be possible to replace all usages of RegisterValidationInterface() with the “Shared” alternative. It is used in:

   • BaseIndex::Start() passing this, is already suggested in #25365 (see last paragraph), #24230 “already does this”.

   • AppInitMain() passing g_zmq_notification_interface which is a global raw ptr, can be changed to global shared_ptr.

   • AppInitMain() passing node.peerman.get() which is unique_ptr. Extracting the raw pointer from unique_ptr, saving it somewhere else and using it from other threads defeats the purpose of using unique_ptr because we must now manually manage the lifetime of the object. NodeContext::peerman should be changed from unique_ptr to shared_ptr.

   This should make it possible to remove RegisterValidationInterface() and UnregisterValidationInterface() (which is already deprecated).

   Chasing concept ACK.

2. vasild added the label Bug on Sep 30, 2022
3. maflcko added the label Refactoring on Sep 30, 2022
4. maflcko removed the label Bug on Sep 30, 2022
5. 
   ryanofsky commented at 12:28 pm on September 30, 2022: contributor

   I’m not sure about this. I think maybe there could be some benefit doing this for ZMQ and peerman. But it also could make ZMQ and peerman implementations messier. A problem with shared_ptr is it can spread virally. Since shared_ptr classes can have unpredictable extended lifetimes, any external state they point to also might need to have extended lifetimes, so any plain pointers or references they contain might need to be converted into shared_ptrs.

   For indexes, instead of making all indexes into shared_ptrs (which would not be a completely trivial change just because all the index init code for 3 index subclasses that needs to be updated), I would really prefer not to make this change because I already implemented code in #24230 to switch from RegisterValidationInterface to RegisterSharedValidationInterface without needing to make the indexes themselves shared_ptrs. Combined with #24230 this would leave the indexes as shared_ptrs without an actual reason for them to be shared_ptrs.

6. 
   ryanofsky commented at 3:00 pm on September 30, 2022: contributor

   shared_ptr exists with the purpose of resolving that problem exactly.

   I don’t actually think shared_ptr is a good solution for allowing clients to unregister from validationinterface generally. It works ok for the wallet, because wallet is already using shared_ptr. But for validationinterface clients that aren’t already using shared_ptr, it means they have to start using it, and still have to deal with complications of having notifications arrive after unregistering, even if they don’t have to deal with the use-after-free complication anymore.

   I also don’t think current solution of having clients call SyncWithValidationInterfaceQueue is optimal either. It works ok as long as clients only need to unregister during shutdown. But if they want to unregister other times this could make them wait for a backlog notifications they don’t actually depend on.

   Just out of curiousity I looked into whether it would be possible to implement a blocking unregister call that would prevent any notification from arriving after unregistering, and it doesn’t look so complicated, so maybe it could be helpful in the future:

     0diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
     1index 613c5b65ef6..4f8e88e13e0 100644
     2--- a/src/validationinterface.cpp
     3+++ b/src/validationinterface.cpp
     4@@ -33,9 +33,18 @@ private:
     5     //! count is equal to the number of current executions of that entry, plus 1
     6     //! if it's registered. It cannot be 0 because that would imply it is
     7     //! unregistered and also not being executed (so shouldn't exist).
     8-    struct ListEntry { std::shared_ptr<CValidationInterface> callbacks; int count = 1; };
     9+    struct ListEntry { std::shared_ptr<CValidationInterface> callbacks; int count = 1; std::function<void()> on_erase; };
    10     std::list<ListEntry> m_list GUARDED_BY(m_mutex);
    11     std::unordered_map<CValidationInterface*, std::list<ListEntry>::iterator> m_map GUARDED_BY(m_mutex);
    12+    //! Condition variable triggered when callback unregistered and freed.
    13+    std::condition_variable m_erased;
    14+
    15+    std::list<ListEntry>::iterator ListErase(std::list<ListEntry>::iterator it) EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
    16+    {
    17+        m_erased.notify_all();
    18+        if (it->on_erase) it->on_erase();
    19+        return m_list.erase(it);
    20+    }
    21 
    22 public:
    23     // We are not allowed to assume the scheduler only runs in one thread,
    24@@ -53,13 +62,20 @@ public:
    25         inserted.first->second->callbacks = std::move(callbacks);
    26     }
    27 
    28-    void Unregister(CValidationInterface* callbacks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
    29+    void Unregister(CValidationInterface* callbacks, bool block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
    30     {
    31-        LOCK(m_mutex);
    32+        WAIT_LOCK(m_mutex, lock);
    33         auto it = m_map.find(callbacks);
    34         if (it != m_map.end()) {
    35-            if (!--it->second->count) m_list.erase(it->second);
    36+            bool free = !--it->second->count;
    37+            if (free) ListErase(it->second);
    38             m_map.erase(it);
    39+            if (!free && block) {
    40+                bool erased = false;
    41+                assert(!it->second->on_erase);
    42+                it->second->on_erase = [&] EXCLUSIVE_LOCKS_REQUIRED(m_mutex) { erased = true; };
    43+                m_erased.wait(lock, [&] { return erased; });
    44+            }
    45         }
    46     }
    47 
    48@@ -71,7 +87,7 @@ public:
    49     {
    50         LOCK(m_mutex);
    51         for (const auto& entry : m_map) {
    52-            if (!--entry.second->count) m_list.erase(entry.second);
    53+            if (!--entry.second->count) ListErase(entry.second);
    54         }
    55         m_map.clear();
    56     }
    57@@ -85,7 +101,7 @@ public:
    58                 REVERSE_LOCK(lock);
    59                 f(*it->callbacks);
    60             }
    61-            it = --it->count ? std::next(it) : m_list.erase(it);
    62+            it = --it->count ? std::next(it) : ListErase(it);
    63         }
    64     }
    65 };
    66@@ -140,10 +156,10 @@ void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> c
    67     UnregisterValidationInterface(callbacks.get());
    68 }
    69 
    70-void UnregisterValidationInterface(CValidationInterface* callbacks)
    71+void UnregisterValidationInterface(CValidationInterface* callbacks, bool block)
    72 {
    73     if (g_signals.m_internals) {
    74-        g_signals.m_internals->Unregister(callbacks);
    75+        g_signals.m_internals->Unregister(callbacks, block);
    76     }
    77 }
    78 
    79diff --git a/src/validationinterface.h b/src/validationinterface.h
    80index a929a3d56bf..75907e89ced 100644
    81--- a/src/validationinterface.h
    82+++ b/src/validationinterface.h
    83@@ -24,7 +24,7 @@ enum class MemPoolRemovalReason;
    84 /** Register subscriber */
    85 void RegisterValidationInterface(CValidationInterface* callbacks);
    86 /** Unregister subscriber. DEPRECATED. This is not safe to use when the RPC server or main message handler thread is running. */
    87-void UnregisterValidationInterface(CValidationInterface* callbacks);
    88+void UnregisterValidationInterface(CValidationInterface* callbacks, bool block = false);
    89 /** Unregister all subscribers */
    90 void UnregisterAllValidationInterfaces();
    91 
    92@@ -181,7 +181,7 @@ private:
    93     std::unique_ptr<MainSignalsImpl> m_internals;
    94 
    95     friend void ::RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>);
    96-    friend void ::UnregisterValidationInterface(CValidationInterface*);
    97+    friend void ::UnregisterValidationInterface(CValidationInterface*, bool);
    98     friend void ::UnregisterAllValidationInterfaces();
    99     friend void ::CallFunctionInValidationInterfaceQueue(std::function<void ()> func);
   100 
   

   I guess I’m not sure what next steps should be. I’d like to move forward with #26215 and #26188 to fix immediate race and TSAN problems with indexes. And I’d like to move forward with #24230 to switch from indexes from using RegisterValidationInterface to RegisterSharedValidationInterface without requiring indexes themselves to be shared_ptrs with unpredictable lifetimes. After that I do think having a blocking UnregisterValidationInterface that stops sending notifications after unregistering could be useful to clean up other problems that may be remaining or that come up in the future.

7. 
   willcl-ark commented at 3:02 pm on October 14, 2024: member

   Closing this as it has not had any activity in a while. If you are interested in continuing discussion on this, or you feel that it is sufficiently important, please leave a comment so that it can be reopened.
8. willcl-ark closed this on Oct 14, 2024

   ---

Contributors
vasild ryanofsky willcl-ark

Labels
Refactoring