From e15b1cfc310df739b92bd281112dbeb31d3bb30a Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 1 Sep 2020 09:40:13 +0200 Subject: [PATCH 1/5] Various cleanups in zmqnotificationinterface. This is a pure refactoring of zmqnotificationinterface to make the code easier to read and maintain. It replaces explicit iterators with C++11 for-each loops where appropriate and uses std::unique_ptr to make memory ownership more explicit. --- src/zmq/zmqnotificationinterface.cpp | 58 +++++++++------------------- src/zmq/zmqnotificationinterface.h | 3 +- src/zmq/zmqpublishnotifier.cpp | 8 ++++ 3 files changed, 28 insertions(+), 41 deletions(-) diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index d55b106e04..96e5b3c039 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -20,33 +20,26 @@ CZMQNotificationInterface::CZMQNotificationInterface() : pcontext(nullptr) CZMQNotificationInterface::~CZMQNotificationInterface() { Shutdown(); - - for (std::list::iterator i=notifiers.begin(); i!=notifiers.end(); ++i) - { - delete *i; - } } std::list CZMQNotificationInterface::GetActiveNotifiers() const { std::list result; - for (const auto* n : notifiers) { - result.push_back(n); + for (const auto& n : notifiers) { + result.push_back(n.get()); } return result; } CZMQNotificationInterface* CZMQNotificationInterface::Create() { - CZMQNotificationInterface* notificationInterface = nullptr; std::map factories; - std::list notifiers; - factories["pubhashblock"] = CZMQAbstractNotifier::Create; factories["pubhashtx"] = CZMQAbstractNotifier::Create; factories["pubrawblock"] = CZMQAbstractNotifier::Create; factories["pubrawtx"] = CZMQAbstractNotifier::Create; + std::list> notifiers; for (const auto& entry : factories) { std::string arg("-zmq" + entry.first); @@ -58,23 +51,21 @@ CZMQNotificationInterface* CZMQNotificationInterface::Create() notifier->SetType(entry.first); notifier->SetAddress(address); notifier->SetOutboundMessageHighWaterMark(static_cast(gArgs.GetArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM))); - notifiers.push_back(notifier); + notifiers.emplace_back(notifier); } } if (!notifiers.empty()) { - notificationInterface = new CZMQNotificationInterface(); - notificationInterface->notifiers = notifiers; + std::unique_ptr notificationInterface(new CZMQNotificationInterface()); + notificationInterface->notifiers = std::move(notifiers); - if (!notificationInterface->Initialize()) - { - delete notificationInterface; - notificationInterface = nullptr; + if (notificationInterface->Initialize()) { + return notificationInterface.release(); } } - return notificationInterface; + return nullptr; } // Called at startup to conditionally set up ZMQ socket(s) @@ -95,26 +86,15 @@ bool CZMQNotificationInterface::Initialize() return false; } - std::list::iterator i=notifiers.begin(); - for (; i!=notifiers.end(); ++i) - { - CZMQAbstractNotifier *notifier = *i; - if (notifier->Initialize(pcontext)) - { + for (auto& notifier : notifiers) { + if (notifier->Initialize(pcontext)) { LogPrint(BCLog::ZMQ, "zmq: Notifier %s ready (address = %s)\n", notifier->GetType(), notifier->GetAddress()); - } - else - { + } else { LogPrint(BCLog::ZMQ, "zmq: Notifier %s failed (address = %s)\n", notifier->GetType(), notifier->GetAddress()); - break; + return false; } } - if (i!=notifiers.end()) - { - return false; - } - return true; } @@ -124,9 +104,7 @@ void CZMQNotificationInterface::Shutdown() LogPrint(BCLog::ZMQ, "zmq: Shutdown notification interface\n"); if (pcontext) { - for (std::list::iterator i=notifiers.begin(); i!=notifiers.end(); ++i) - { - CZMQAbstractNotifier *notifier = *i; + for (auto& notifier : notifiers) { LogPrint(BCLog::ZMQ, "zmq: Shutdown notifier %s at %s\n", notifier->GetType(), notifier->GetAddress()); notifier->Shutdown(); } @@ -141,9 +119,9 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co if (fInitialDownload || pindexNew == pindexFork) // In IBD or blocks were disconnected without any new ones return; - for (std::list::iterator i = notifiers.begin(); i!=notifiers.end(); ) + for (auto i = notifiers.begin(); i!=notifiers.end(); ) { - CZMQAbstractNotifier *notifier = *i; + CZMQAbstractNotifier *notifier = i->get(); if (notifier->NotifyBlock(pindexNew)) { i++; @@ -162,9 +140,9 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& // all the same external callback. const CTransaction& tx = *ptx; - for (std::list::iterator i = notifiers.begin(); i!=notifiers.end(); ) + for (auto i = notifiers.begin(); i!=notifiers.end(); ) { - CZMQAbstractNotifier *notifier = *i; + CZMQAbstractNotifier *notifier = i->get(); if (notifier->NotifyTransaction(tx)) { i++; diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 60f3b6148a..0686960ed4 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -7,6 +7,7 @@ #include #include +#include class CBlockIndex; class CZMQAbstractNotifier; @@ -34,7 +35,7 @@ private: CZMQNotificationInterface(); void *pcontext; - std::list notifiers; + std::list> notifiers; }; extern CZMQNotificationInterface* g_zmq_notification_interface; diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index e2431cbbb7..aaef12ebcf 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -10,6 +10,14 @@ #include #include +#include + +#include +#include +#include +#include +#include + static std::multimap mapPublishNotifiers; static const char *MSG_HASHBLOCK = "hashblock"; From b93b9d54569145bfcec6cee10968284fe05fe254 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 1 Sep 2020 09:40:43 +0200 Subject: [PATCH 2/5] Simplify and fix notifier removal on error. This factors out the common logic to run over all ZMQ notifiers, call a function on them, and remove them from the list if the function fails is extracted to a helper method. Note that this also fixes a potential memory leak: When a notifier was removed previously after its callback returned false, it would just be removed from the list without destructing the object. This is now done correctly by std::unique_ptr behind the scenes. --- src/zmq/zmqnotificationinterface.cpp | 50 +++++++++++++--------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 96e5b3c039..449651e65f 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -114,24 +114,32 @@ void CZMQNotificationInterface::Shutdown() } } +namespace { + +template +void TryForEachAndRemoveFailed(std::list>& notifiers, const Function& func) +{ + for (auto i = notifiers.begin(); i != notifiers.end(); ) { + CZMQAbstractNotifier* notifier = i->get(); + if (func(notifier)) { + ++i; + } else { + notifier->Shutdown(); + i = notifiers.erase(i); + } + } +} + +} // anonymous namespace + void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { if (fInitialDownload || pindexNew == pindexFork) // In IBD or blocks were disconnected without any new ones return; - for (auto i = notifiers.begin(); i!=notifiers.end(); ) - { - CZMQAbstractNotifier *notifier = i->get(); - if (notifier->NotifyBlock(pindexNew)) - { - i++; - } - else - { - notifier->Shutdown(); - i = notifiers.erase(i); - } - } + TryForEachAndRemoveFailed(notifiers, [pindexNew](CZMQAbstractNotifier* notifier) { + return notifier->NotifyBlock(pindexNew); + }); } void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx) @@ -140,19 +148,9 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& // all the same external callback. const CTransaction& tx = *ptx; - for (auto i = notifiers.begin(); i!=notifiers.end(); ) - { - CZMQAbstractNotifier *notifier = i->get(); - if (notifier->NotifyTransaction(tx)) - { - i++; - } - else - { - notifier->Shutdown(); - i = notifiers.erase(i); - } - } + TryForEachAndRemoveFailed(notifiers, [&tx](CZMQAbstractNotifier* notifier) { + return notifier->NotifyTransaction(tx); + }); } void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) From 7f2ad1b9acef4ccc1b3e1a9f551416235d95cbfd Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 17 Jul 2018 12:36:22 +0200 Subject: [PATCH 3/5] Use std::unique_ptr for CZMQNotifierFactory. Instead of returning a raw pointer from CZMQNotifierFactory and implicitly requiring the caller to know that it has to take ownership, return a std::unique_ptr to make this explicit. This also changes the typedef for CZMQNotifierFactory to use the new C++11 using syntax, which makes it (a little) less cryptic. --- src/zmq/zmqabstractnotifier.h | 10 +++++++--- src/zmq/zmqnotificationinterface.cpp | 8 ++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/zmq/zmqabstractnotifier.h b/src/zmq/zmqabstractnotifier.h index 887dde7b27..8377a26d3a 100644 --- a/src/zmq/zmqabstractnotifier.h +++ b/src/zmq/zmqabstractnotifier.h @@ -7,10 +7,14 @@ #include +#include + +#include + class CBlockIndex; class CZMQAbstractNotifier; -typedef CZMQAbstractNotifier* (*CZMQNotifierFactory)(); +using CZMQNotifierFactory = std::unique_ptr (*)(); class CZMQAbstractNotifier { @@ -21,9 +25,9 @@ public: virtual ~CZMQAbstractNotifier(); template - static CZMQAbstractNotifier* Create() + static std::unique_ptr Create() { - return new T(); + return MakeUnique(); } std::string GetType() const { return type; } diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 449651e65f..9e9acdc568 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -45,13 +45,13 @@ CZMQNotificationInterface* CZMQNotificationInterface::Create() std::string arg("-zmq" + entry.first); if (gArgs.IsArgSet(arg)) { - CZMQNotifierFactory factory = entry.second; - std::string address = gArgs.GetArg(arg, ""); - CZMQAbstractNotifier *notifier = factory(); + const auto& factory = entry.second; + const std::string address = gArgs.GetArg(arg, ""); + std::unique_ptr notifier = factory(); notifier->SetType(entry.first); notifier->SetAddress(address); notifier->SetOutboundMessageHighWaterMark(static_cast(gArgs.GetArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM))); - notifiers.emplace_back(notifier); + notifiers.push_back(std::move(notifier)); } } From a3ffb6ebebd753cec294c91cef7c603a30cf217e Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 17 Jul 2018 12:51:23 +0200 Subject: [PATCH 4/5] Replace zmqconfig.h by a simple zmqutil. zmqconfig.h is currently not really needed anywhere, except that it declares zmqError (which is then defined in zmqnotificationinterface.cpp). Note in particular that there is no need to conditionally include zmq.h only if ZMQ is enabled, because the place in the core code where the ZMQ library itself is included (init.cpp) is conditional already on that. This commit removes zmqconfig.h and replaces it by a much simpler zmqutil.h library for zmqError. The definition of the function is moved to the matching (newly created) zmqutil.cpp. --- src/Makefile.am | 7 ++++--- src/zmq/zmqabstractnotifier.cpp | 2 ++ src/zmq/zmqabstractnotifier.h | 4 ++-- src/zmq/zmqconfig.h | 22 ---------------------- src/zmq/zmqnotificationinterface.cpp | 8 +++----- src/zmq/zmqpublishnotifier.cpp | 10 ++++++---- src/zmq/zmqutil.cpp | 14 ++++++++++++++ src/zmq/zmqutil.h | 10 ++++++++++ 8 files changed, 41 insertions(+), 36 deletions(-) delete mode 100644 src/zmq/zmqconfig.h create mode 100644 src/zmq/zmqutil.cpp create mode 100644 src/zmq/zmqutil.h diff --git a/src/Makefile.am b/src/Makefile.am index 175501d4a6..61ae49df31 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -262,10 +262,10 @@ BITCOIN_CORE_H = \ walletinitinterface.h \ warnings.h \ zmq/zmqabstractnotifier.h \ - zmq/zmqconfig.h\ zmq/zmqnotificationinterface.h \ zmq/zmqpublishnotifier.h \ - zmq/zmqrpc.h + zmq/zmqrpc.h \ + zmq/zmqutil.h obj/build.h: FORCE @@ -344,7 +344,8 @@ libbitcoin_zmq_a_SOURCES = \ zmq/zmqabstractnotifier.cpp \ zmq/zmqnotificationinterface.cpp \ zmq/zmqpublishnotifier.cpp \ - zmq/zmqrpc.cpp + zmq/zmqrpc.cpp \ + zmq/zmqutil.cpp endif diff --git a/src/zmq/zmqabstractnotifier.cpp b/src/zmq/zmqabstractnotifier.cpp index aae760adde..0d0428f3c0 100644 --- a/src/zmq/zmqabstractnotifier.cpp +++ b/src/zmq/zmqabstractnotifier.cpp @@ -4,6 +4,8 @@ #include +#include + const int CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM; CZMQAbstractNotifier::~CZMQAbstractNotifier() diff --git a/src/zmq/zmqabstractnotifier.h b/src/zmq/zmqabstractnotifier.h index 8377a26d3a..34d7e5ef03 100644 --- a/src/zmq/zmqabstractnotifier.h +++ b/src/zmq/zmqabstractnotifier.h @@ -5,13 +5,13 @@ #ifndef BITCOIN_ZMQ_ZMQABSTRACTNOTIFIER_H #define BITCOIN_ZMQ_ZMQABSTRACTNOTIFIER_H -#include - #include #include +#include class CBlockIndex; +class CTransaction; class CZMQAbstractNotifier; using CZMQNotifierFactory = std::unique_ptr (*)(); diff --git a/src/zmq/zmqconfig.h b/src/zmq/zmqconfig.h deleted file mode 100644 index 5f0036206d..0000000000 --- a/src/zmq/zmqconfig.h +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) 2014-2019 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#ifndef BITCOIN_ZMQ_ZMQCONFIG_H -#define BITCOIN_ZMQ_ZMQCONFIG_H - -#if defined(HAVE_CONFIG_H) -#include -#endif - -#include - -#if ENABLE_ZMQ -#include -#endif - -#include - -void zmqError(const char *str); - -#endif // BITCOIN_ZMQ_ZMQCONFIG_H diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 9e9acdc568..a22772baed 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -4,15 +4,13 @@ #include #include +#include + +#include #include #include -void zmqError(const char *str) -{ - LogPrint(BCLog::ZMQ, "zmq: Error: %s, errno=%s\n", str, zmq_strerror(errno)); -} - CZMQNotificationInterface::CZMQNotificationInterface() : pcontext(nullptr) { } diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index aaef12ebcf..2484fd6988 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -2,13 +2,15 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include + #include #include -#include -#include -#include -#include #include +#include +#include +#include +#include #include diff --git a/src/zmq/zmqutil.cpp b/src/zmq/zmqutil.cpp new file mode 100644 index 0000000000..f07a4ae9fd --- /dev/null +++ b/src/zmq/zmqutil.cpp @@ -0,0 +1,14 @@ +// Copyright (c) 2014-2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include + +#include + +void zmqError(const char* str) +{ + LogPrint(BCLog::ZMQ, "zmq: Error: %s, errno=%s\n", str, zmq_strerror(errno)); +} diff --git a/src/zmq/zmqutil.h b/src/zmq/zmqutil.h new file mode 100644 index 0000000000..4c1df5d6db --- /dev/null +++ b/src/zmq/zmqutil.h @@ -0,0 +1,10 @@ +// Copyright (c) 2014-2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_ZMQ_ZMQUTIL_H +#define BITCOIN_ZMQ_ZMQUTIL_H + +void zmqError(const char* str); + +#endif // BITCOIN_ZMQ_ZMQUTIL_H From 6fe2ef2acb00b1df7f6a0c0dea1a81a1924be0e1 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Sat, 10 Nov 2018 20:05:34 +0000 Subject: [PATCH 5/5] scripted-diff: Rename SendMessage to SendZmqMessage. Windows headers define SendMessage as a macro, which leads to problems with the method name "SendMessage". To circumvent this, we rename the method to "SendZmqMessage". -BEGIN VERIFY SCRIPT- sed -i 's/SendMessage/SendZmqMessage/g' src/zmq/zmqpublishnotifier.* -END VERIFY SCRIPT- --- src/zmq/zmqpublishnotifier.cpp | 10 +++++----- src/zmq/zmqpublishnotifier.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index 2484fd6988..d4d21b05ba 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -159,7 +159,7 @@ void CZMQAbstractPublishNotifier::Shutdown() psocket = nullptr; } -bool CZMQAbstractPublishNotifier::SendMessage(const char *command, const void* data, size_t size) +bool CZMQAbstractPublishNotifier::SendZmqMessage(const char *command, const void* data, size_t size) { assert(psocket); @@ -183,7 +183,7 @@ bool CZMQPublishHashBlockNotifier::NotifyBlock(const CBlockIndex *pindex) char data[32]; for (unsigned int i = 0; i < 32; i++) data[31 - i] = hash.begin()[i]; - return SendMessage(MSG_HASHBLOCK, data, 32); + return SendZmqMessage(MSG_HASHBLOCK, data, 32); } bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &transaction) @@ -193,7 +193,7 @@ bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &t char data[32]; for (unsigned int i = 0; i < 32; i++) data[31 - i] = hash.begin()[i]; - return SendMessage(MSG_HASHTX, data, 32); + return SendZmqMessage(MSG_HASHTX, data, 32); } bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex) @@ -214,7 +214,7 @@ bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex) ss << block; } - return SendMessage(MSG_RAWBLOCK, &(*ss.begin()), ss.size()); + return SendZmqMessage(MSG_RAWBLOCK, &(*ss.begin()), ss.size()); } bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &transaction) @@ -223,5 +223,5 @@ bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &tr LogPrint(BCLog::ZMQ, "zmq: Publish rawtx %s\n", hash.GetHex()); CDataStream ss(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags()); ss << transaction; - return SendMessage(MSG_RAWTX, &(*ss.begin()), ss.size()); + return SendZmqMessage(MSG_RAWTX, &(*ss.begin()), ss.size()); } diff --git a/src/zmq/zmqpublishnotifier.h b/src/zmq/zmqpublishnotifier.h index 278fdb94d2..eb9ae881be 100644 --- a/src/zmq/zmqpublishnotifier.h +++ b/src/zmq/zmqpublishnotifier.h @@ -22,7 +22,7 @@ public: * data * message sequence number */ - bool SendMessage(const char *command, const void* data, size_t size); + bool SendZmqMessage(const char *command, const void* data, size_t size); bool Initialize(void *pcontext) override; void Shutdown() override;