CValidationInterface: ValidationInterfaceUnregistering, called when being unregistered

luke-jr commented at 7:04 pm on May 1, 2022: member

Part of https://github.com/bitcoin-core/gui/pull/444 (GUI Network Watch tool)

CValidationInterface: ValidationInterfaceUnregistering, called when being unregistered 8eed7dd722

luke-jr force-pushed on May 1, 2022

DrahtBot added the label Validation on May 1, 2022

vincenzopalazzo commented at 8:23 pm on May 3, 2022: none

Concept ACK

pk-b2 commented at 6:03 pm on May 4, 2022: none

Concept ACK 8eed7dd722177c2e36645d66e2c8254f9037a0e3

in src/validationinterface.cpp:142 in 8eed7dd722

138@@ -139,6 +139,8 @@ void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> c
139 
140 void UnregisterValidationInterface(CValidationInterface* callbacks)
141 {
142+    callbacks->ValidationInterfaceUnregistering();

pk-b2 commented at 6:06 pm on May 4, 2022:

Is it worth to be more specific in the naming whether the unregistering will happen or has already happened?

Like renaming the function to ::ValidationInterfaceWillUnregister?

This makes it clearer that the callback hasn’t been removed yet.

Comparing to introducing a callback which gets called after the fact of the callback being removed `::ValidationInterfaceDidUnregister'

in src/validationinterface.cpp:155 in 8eed7dd722

150@@ -149,6 +151,9 @@ void UnregisterAllValidationInterfaces()
151     if (!g_signals.m_internals) {
152         return;
153     }
154+
155+    g_signals.m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ValidationInterfaceUnregistering(); });

pk-b2 commented at 6:07 pm on May 4, 2022:

👍 Follows the same approach as in ::UnregisterValidationInterface of being called before the callback gets removed from the list

in src/validationinterface.h:179 in 8eed7dd722

172@@ -173,6 +173,13 @@ class CValidationInterface {
173      * Notifies listeners that a block which builds directly on our current tip
174      * has been received and connected to the headers tree, though not validated yet */
175     virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) {};
176+    /**
177+     * Notifies the validation interface that it is being unregistered
178+     */
179+    virtual void ValidationInterfaceUnregistering() {};

pk-b2 commented at 6:08 pm on May 4, 2022:

You don’t need it in https://github.com/bitcoin-core/gui/pull/444 but is it worth to consider to add ::ValidationInterfaceRegistering as part of this PR for completeness?

in src/validationinterface.h:181 in 8eed7dd722

172@@ -173,6 +173,13 @@ class CValidationInterface {
173      * Notifies listeners that a block which builds directly on our current tip
174      * has been received and connected to the headers tree, though not validated yet */
175     virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) {};
176+    /**
177+     * Notifies the validation interface that it is being unregistered
178+     */
179+    virtual void ValidationInterfaceUnregistering() {};
180+
181+    friend void UnregisterValidationInterface(CValidationInterface*);

jonatack commented at 3:05 pm on May 5, 2022:

This is a deprecated method due to being racy, according to the documentation added in fa770ce7fe67685c43780e219d8232efbee0bb8e (see #18742). @MarcoFalke, thoughts?

0+/** Unregister subscriber. DEPRECATED. This is not safe to use when the RPC server or main message handler thread is running. */
1+void UnregisterValidationInterface(CValidationInterface* callbacks);

jonatack commented at 3:17 pm on May 5, 2022: contributor

Light ACK 8eed7dd722177c2e36645d66e2c8254f9037a0e3

Modulo a question below.

DrahtBot commented at 12:15 pm on September 23, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack
Concept ACK	vincenzopalazzo, pk-b2

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

ryanofsky commented at 5:32 pm on October 6, 2022: contributor

I don’t understand what this is doing in the context of https://github.com/bitcoin-core/gui/pull/444. It looks like the NetWatchLogModel::~NetWatchLogModel destructor there calls UnregisterValidationInterface, which (because of the change in this PR) calls NetWatchValidationInterface::ValidationInterfaceUnregistering which calls NetWatchLogModel::OrphanedValidationInterface which then does delete m_validation_interface before the ~NetWatchLogModel repeats delete m_validation_interface again which is a no-op.

But maybe the point of this is not actually to hook the UnregisterValidationInterface call but the UnregisterAllValidationInterfaces call? I’m not sure why that would be necessary either though. If the UnregisterAllValidationInterfaces call is happening before the ~NetWatchLogModel destructor, it seems harmless but pointless to delete the validationinterface early. If destructor runs after UnregisterAll, the destructor is already unregistering immediately so there would no UnregisterAll callback would happen at all.

So I’m confused what new callback is doing and think PR could benefit from a description.

Probably also in #444 GUI code shouldn’t be making low level-validationinterface calls. Ideally it would use something like the interfaces::Node::handleNotifyBlockTip function or interfaces::Chain::Notifications::updatedBlockTip and transactionAddedToMempool functions but that is probably tangential to whatever issue this PR is solving.

fanquake commented at 10:17 am on December 5, 2022: member

@luke-jr are you planning on either adding to the PR description to better explain the changes, or answering the review questions left here?

fanquake commented at 2:45 pm on February 6, 2023: member

No response to review questions. Still no PR description. Can be re-opened if that is going to change.

fanquake closed this on Feb 6, 2023

bitcoin locked this on Feb 6, 2024

CValidationInterface: ValidationInterfaceUnregistering, called when being unregistered #25050

Reviews

Conflicts