Add missing virtual destructor in PeerLogicValidation #12680

Silence the following compiler warning:

/usr/include/c++/v1/memory:2285:5: error: delete called on non-final 'PeerLogicValidation' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor] delete __ptr; ^ /usr/include/c++/v1/memory:2598:7: note: in instantiation of member function 'std::__1::default_delete<PeerLogicValidation>::operator()' requested here __ptr_.second()(__tmp); ^ init.cpp:201:15: note: in instantiation of member function 'std::__1::unique_ptr<PeerLogicValidation, std::__1::default_delete<PeerLogicValidation> >::reset' requested here peerLogic.reset(); ^

utACK 0d58b577dd00df9fb15b71f77ab59545a5a1f95c

Thanks for fixing this annoying warning!

utACK 0d58b57

It seems strange to fix this in the derived class.

If a virtual destructor is added to CValidationInterface and CNetEventsInterface instead it will automatically fix it for all possible future derived classes for these interfaces.

Edit: responded before seeing @sipa's comment. That would work as well, I'd just rather go ahead and make PeerLogicValidation explicitly non-derivable, as only bad things could come from doing that :)

The issue here isn't PeerLogicValidation's lack of virtual destructor, it's that it's possible to delete a PeerLogicValidation through a base CValidationInterface or NetEventsInterface pointer, or even something derived from PeerLogicValidation.

Making PeerLogicValidation's destructor virtual would only muddy the situation further, making it appear as though it's meant to be derived-from. Instead, I'd rather we address the actual issue by making sure that a PeerLogicValidation can't be deleted through a base or derived class.

We can address that, while potentially benefiting from extra devirtualization, by marking the interface destructors as protected, and the derived class as final. That should also shut clang up :)

--- a/src/net.h
+++ b/src/net.h
@@ -466,11 +466,17 @@ struct CombinerAll
  */
 class NetEventsInterface
 {
-public:
+protected:
     virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
     virtual bool SendMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
     virtual void InitializeNode(CNode* pnode) = 0;
     virtual void FinalizeNode(NodeId id, bool& update_connection_time) = 0;
+    /**
+     *
+     * Protected destructor so that instances can only be deleted by derived classes.
+     * If that restriction is no longer desired, this should be made public and virtual.
+     */
+    ~NetEventsInterface() = default;
 };

 enum
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -35,7 +35,7 @@ static constexpr int64_t EXTRA_PEER_CHECK_INTERVAL = 45;
 /** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict, in seconds */
 static constexpr int64_t MINIMUM_CONNECT_TIME = 30;

-class PeerLogicValidation : public CValidationInterface, public NetEventsInterface {
+class PeerLogicValidation final : public CValidationInterface, public NetEventsInterface {
 private:
     CConnman* const connman;

--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -56,6 +56,12 @@ void SyncWithValidationInterfaceQueue();
 class CValidationInterface {
 protected:
     /**
+     *
+     * Protected destructor so that instances can only be deleted by derived classes.
+     * If that restriction is no longer desired, this should be made public and virtual.
+     */
+    ~CValidationInterface() = default;
+    /**
      * Notifies listeners of updated block chain tip
      *
      * Called on a background thread.

Allright, removed Trivial: from the pull request subject and pushed a new patch.

utACK 2b3ea39

The issue here isn't PeerLogicValidation's lack of virtual destructor, it's that it's possible to delete a PeerLogicValidation through a base CValidationInterface or NetEventsInterface pointer, or even something derived from PeerLogicValidation.

TIL. I thought that was a necessity for base classes w/ virtual methods. But avoiding this behavior if it's unused is certainly better.

utACK 2b3ea39de40bc7754cab558245e4ddac1b261750