zmq: Log bind error at Error level, abort startup on init error

isrod commented at 6:59 pm on October 28, 2025: none

Fixes #33715

DrahtBot commented at 6:59 pm on October 28, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33727.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stickies-v, pinheadmz

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

A-Manning force-pushed on Oct 28, 2025

in src/zmq/zmqnotificationinterface.h:30 in 759725ffed outdated

25@@ -26,7 +26,8 @@ class CZMQNotificationInterface final : public CValidationInterface
26 
27     std::list<const CZMQAbstractNotifier*> GetActiveNotifiers() const;
28 
29-    static std::unique_ptr<CZMQNotificationInterface> Create(std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index);
30+    // nullopt if initialization fails
31+    static std::optional<std::unique_ptr<CZMQNotificationInterface>> Create(std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index);

maflcko commented at 9:31 pm on October 28, 2025:

pointers are nullable, so why wrap twice?

A-Manning commented at 10:34 pm on October 28, 2025:

nullptr is already used to indicate that no ZMQ interface was initialized. This is not the same as failing to initialize a ZMQ interface.

stickies-v commented at 12:20 pm on October 31, 2025:

I also don’t like this double wrapping, it’s error prone. It seems like this is currently necessary because Create is actually more like a MaybeCreate, with actual creation depending on gArgs. A more robust approach imo would be to take gArgs out of Create, let the caller decide if we actually want to create a CZMQNotificationInterface and pass the necessary parameters. With that change, Create should always return a value, so we can keep nullptr for the failure case?

One approach could be to carve out a GetNotifiers function:

  0diff --git a/src/init.cpp b/src/init.cpp
  1index cd3512f3a9..0582d7ed8f 100644
  2--- a/src/init.cpp
  3+++ b/src/init.cpp
  4@@ -1749,15 +1749,18 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
  5     }
  6 
  7 #ifdef ENABLE_ZMQ
  8-    auto zmq_notification_interface = CZMQNotificationInterface::Create(
  9+    auto notifiers{CZMQNotificationInterface::GetNotifiers(
 10+        gArgs,
 11         [&chainman = node.chainman](std::vector<std::byte>& block, const CBlockIndex& index) {
 12             assert(chainman);
 13             return chainman->m_blockman.ReadRawBlock(block, WITH_LOCK(cs_main, return index.GetBlockPos()));
 14-        });
 15-    if (zmq_notification_interface.has_value()) {
 16-        g_zmq_notification_interface = std::move(zmq_notification_interface.value());
 17-    } else {
 18-        return InitError(Untranslated("Initializing ZMQ interface failed."));
 19+        })};
 20+    if (!notifiers.empty()) {
 21+        if (auto zmq_notification_interface{CZMQNotificationInterface::Create(std::move(notifiers))}) {
 22+            g_zmq_notification_interface = std::move(zmq_notification_interface);
 23+        } else {
 24+            return InitError(Untranslated("Initializing ZMQ interface failed."));
 25+        }
 26     }
 27 
 28     if (g_zmq_notification_interface) {
 29diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp
 30index eae377f718..c1e45e33e5 100644
 31--- a/src/zmq/zmqnotificationinterface.cpp
 32+++ b/src/zmq/zmqnotificationinterface.cpp
 33@@ -40,7 +40,9 @@ std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotif
 34     return result;
 35 }
 36 
 37-std::optional<std::unique_ptr<CZMQNotificationInterface>> CZMQNotificationInterface::Create(std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index)
 38+std::list<std::unique_ptr<CZMQAbstractNotifier>> CZMQNotificationInterface::GetNotifiers(
 39+    const ArgsManager& args,
 40+    std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index)
 41 {
 42     std::map<std::string, CZMQNotifierFactory> factories;
 43     factories["pubhashblock"] = CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>;
 44@@ -56,7 +58,7 @@ std::optional<std::unique_ptr<CZMQNotificationInterface>> CZMQNotificationInterf
 45     {
 46         std::string arg("-zmq" + entry.first);
 47         const auto& factory = entry.second;
 48-        for (std::string& address : gArgs.GetArgs(arg)) {
 49+        for (std::string& address : args.GetArgs(arg)) {
 50             // libzmq uses prefix "ipc://" for UNIX domain sockets
 51             if (address.starts_with(ADDR_PREFIX_UNIX)) {
 52                 address.replace(0, ADDR_PREFIX_UNIX.length(), ADDR_PREFIX_IPC);
 53@@ -65,24 +67,26 @@ std::optional<std::unique_ptr<CZMQNotificationInterface>> CZMQNotificationInterf
 54             std::unique_ptr<CZMQAbstractNotifier> notifier = factory();
 55             notifier->SetType(entry.first);
 56             notifier->SetAddress(address);
 57-            notifier->SetOutboundMessageHighWaterMark(static_cast<int>(gArgs.GetIntArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM)));
 58+            notifier->SetOutboundMessageHighWaterMark(static_cast<int>(args.GetIntArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM)));
 59             notifiers.push_back(std::move(notifier));
 60         }
 61     }
 62+    return notifiers;
 63+}
 64 
 65-    if (!notifiers.empty())
 66-    {
 67-        std::unique_ptr<CZMQNotificationInterface> notificationInterface(new CZMQNotificationInterface());
 68-        notificationInterface->notifiers = std::move(notifiers);
 69+std::unique_ptr<CZMQNotificationInterface> CZMQNotificationInterface::Create(
 70+    std::list<std::unique_ptr<CZMQAbstractNotifier>>&& notifiers)
 71+{
 72+    if (notifiers.empty()) return nullptr;
 73 
 74-        if (notificationInterface->Initialize()) {
 75-            return notificationInterface;
 76-        } else {
 77-            return std::nullopt;
 78-        }
 79-    }
 80+    std::unique_ptr<CZMQNotificationInterface> notificationInterface(new CZMQNotificationInterface());
 81+    notificationInterface->notifiers = std::move(notifiers);
 82 
 83-    return nullptr;
 84+    if (notificationInterface->Initialize()) {
 85+        return notificationInterface;
 86+    } else {
 87+        return nullptr;
 88+    }
 89 }
 90 
 91 // Called at startup to conditionally set up ZMQ socket(s)
 92diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h
 93index 0abbbef428..f36d633814 100644
 94--- a/src/zmq/zmqnotificationinterface.h
 95+++ b/src/zmq/zmqnotificationinterface.h
 96@@ -14,6 +14,7 @@
 97 #include <memory>
 98 #include <vector>
 99 
100+class ArgsManager;
101 class CBlock;
102 class CBlockIndex;
103 class CZMQAbstractNotifier;
104@@ -26,8 +27,11 @@ public:
105 
106     std::list<const CZMQAbstractNotifier*> GetActiveNotifiers() const;
107 
108-    // nullopt if initialization fails
109-    static std::optional<std::unique_ptr<CZMQNotificationInterface>> Create(std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index);
110+    static std::list<std::unique_ptr<CZMQAbstractNotifier>> GetNotifiers(
111+        const ArgsManager& args,
112+        std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index);
113+    // nullptr if initialization fails
114+    static std::unique_ptr<CZMQNotificationInterface> Create(std::list<std::unique_ptr<CZMQAbstractNotifier>>&& notifiers);
115 
116 protected:
117     bool Initialize();

Edit: adding some unit tests around those functions could also be helpful.

A-Manning commented at 4:43 pm on October 31, 2025:

Applied the patch, thanks

in src/zmq/zmqnotificationinterface.cpp:43 in 759725ffed outdated

39@@ -40,7 +40,7 @@ std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotif
40     return result;
41 }
42 
43-std::unique_ptr<CZMQNotificationInterface> CZMQNotificationInterface::Create(std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index)
44+std::optional<std::unique_ptr<CZMQNotificationInterface>> CZMQNotificationInterface::Create(std::function<bool(std::vector<std::byte>&, const CBlockIndex&)> get_block_by_index)

stickies-v commented at 10:11 am on October 31, 2025:

nit: line length (also in .h)

in src/zmq/zmqutil.cpp:18 in 759725ffed outdated

15 {
16-    LogDebug(BCLog::ZMQ, "Error: %s, msg: %s\n", str, zmq_strerror(errno));
17+    LogPrintLevel(BCLog::ZMQ, level, "Error: %s, msg: %s\n", str, zmq_strerror(errno));
18+}
19+
20+void zmqErrorDebug(const std::string& str)

stickies-v commented at 10:13 am on October 31, 2025:

Error and Debug are different levels, so this name seems confusing to me. Would simplify to zmqError and zmqDebug, removing the level parameter.

Side note: I don’t know much about zmq, but it seems like some of these debug messages should actually be warnings, e.g. when block gets failed to read from disk? Can/should probably be done in a separate pull, though.

A-Manning commented at 3:21 pm on October 31, 2025:

I agree that zmqDebug/zmqError would be ideal, however zmqError is currently used to log at the Debug level - changing it to log at Error level while keeping the type signature the same seems likely to cause misuse.

stickies-v commented at 11:29 am on November 3, 2025:

This code isn’t touched much, the last commit touching zmqError is from 2021. I don’t think any authors or reviewers would assume zmqError means zmqDebug without looking at the underlying code.

On further inspection, it seems like none of these messages should be debug to begin with? They all look like warning or error to me? Finally, LogZmq{Error,Warning} would be a better name.

Suggested (untested) diff to address all:

  0diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp
  1index c1e45e33e5..85b8b995b9 100644
  2--- a/src/zmq/zmqnotificationinterface.cpp
  3+++ b/src/zmq/zmqnotificationinterface.cpp
  4@@ -103,7 +103,7 @@ bool CZMQNotificationInterface::Initialize()
  5 
  6     if (!pcontext)
  7     {
  8-        zmqErrorDebug("Unable to initialize context");
  9+        LogZmqError("Unable to initialize context");
 10         return false;
 11     }
 12 
 13diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp
 14index 665f4a42f8..a7cbf48915 100644
 15--- a/src/zmq/zmqpublishnotifier.cpp
 16+++ b/src/zmq/zmqpublishnotifier.cpp
 17@@ -59,7 +59,7 @@ static int zmq_send_multipart(void *sock, const void* data, size_t size, ...)
 18         int rc = zmq_msg_init_size(&msg, size);
 19         if (rc != 0)
 20         {
 21-            zmqErrorDebug("Unable to initialize ZMQ msg");
 22+            LogZmqWarning("Unable to initialize ZMQ msg");
 23             va_end(args);
 24             return -1;
 25         }
 26@@ -72,7 +72,7 @@ static int zmq_send_multipart(void *sock, const void* data, size_t size, ...)
 27         rc = zmq_msg_send(&msg, sock, data ? ZMQ_SNDMORE : 0);
 28         if (rc == -1)
 29         {
 30-            zmqErrorDebug("Unable to send ZMQ msg");
 31+            LogZmqWarning("Unable to send ZMQ msg");
 32             zmq_msg_close(&msg);
 33             va_end(args);
 34             return -1;
 35@@ -114,7 +114,7 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
 36         psocket = zmq_socket(pcontext, ZMQ_PUB);
 37         if (!psocket)
 38         {
 39-            zmqErrorDebug("Failed to create socket");
 40+            LogZmqError("Failed to create socket");
 41             return false;
 42         }
 43 
 44@@ -123,7 +123,7 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
 45         int rc = zmq_setsockopt(psocket, ZMQ_SNDHWM, &outbound_message_high_water_mark, sizeof(outbound_message_high_water_mark));
 46         if (rc != 0)
 47         {
 48-            zmqErrorDebug("Failed to set outbound message high water mark");
 49+            LogZmqError("Failed to set outbound message high water mark");
 50             zmq_close(psocket);
 51             return false;
 52         }
 53@@ -131,7 +131,7 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
 54         const int so_keepalive_option {1};
 55         rc = zmq_setsockopt(psocket, ZMQ_TCP_KEEPALIVE, &so_keepalive_option, sizeof(so_keepalive_option));
 56         if (rc != 0) {
 57-            zmqErrorDebug("Failed to set SO_KEEPALIVE");
 58+            LogZmqError("Failed to set SO_KEEPALIVE");
 59             zmq_close(psocket);
 60             return false;
 61         }
 62@@ -140,7 +140,7 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
 63         const int enable_ipv6 { IsZMQAddressIPV6(address) ? 1 : 0};
 64         rc = zmq_setsockopt(psocket, ZMQ_IPV6, &enable_ipv6, sizeof(enable_ipv6));
 65         if (rc != 0) {
 66-            zmqErrorDebug("Failed to set ZMQ_IPV6");
 67+            LogZmqError("Failed to set ZMQ_IPV6");
 68             zmq_close(psocket);
 69             return false;
 70         }
 71@@ -148,7 +148,7 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
 72         rc = zmq_bind(psocket, address.c_str());
 73         if (rc != 0)
 74         {
 75-            zmqError(BCLog::Level::Error, "Failed to bind address");
 76+            LogZmqError("Failed to bind address");
 77             zmq_close(psocket);
 78             return false;
 79         }
 80@@ -245,7 +245,7 @@ bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex)
 81 
 82     std::vector<std::byte> block{};
 83     if (!m_get_block_by_index(block, *pindex)) {
 84-        zmqErrorDebug("Can't read block from disk");
 85+        LogZmqWarning("Can't read block from disk");
 86         return false;
 87     }
 88 
 89diff --git a/src/zmq/zmqutil.cpp b/src/zmq/zmqutil.cpp
 90index 86d0b5a2cf..e10817ce19 100644
 91--- a/src/zmq/zmqutil.cpp
 92+++ b/src/zmq/zmqutil.cpp
 93@@ -8,14 +8,18 @@
 94 #include <zmq.h>
 95 
 96 #include <cerrno>
 97-#include <string>
 98+#include <string_view>
 99 
100-void zmqError(BCLog::Level level, const std::string& str)
101+void LogZmqError(std::string_view context)
102 {
103-    LogPrintLevel(BCLog::ZMQ, level, "Error: %s, msg: %s\n", str, zmq_strerror(errno));
104+    LogPrintLevel(BCLog::ZMQ, BCLog::Level::Error,
105+                  "%s, zmq_errno: %d, msg: %s\n",
106+                  context, errno, zmq_strerror(errno));
107 }
108 
109-void zmqErrorDebug(const std::string& str)
110+void LogZmqWarning(std::string_view context)
111 {
112-    zmqError(BCLog::Level::Debug, str);
113+    LogPrintLevel(BCLog::ZMQ, BCLog::Level::Warning,
114+                  "%s, zmq_errno: %d, msg: %s\n",
115+                  context, errno, zmq_strerror(errno));
116 }
117diff --git a/src/zmq/zmqutil.h b/src/zmq/zmqutil.h
118index 595e2c1d31..115254e5f4 100644
119--- a/src/zmq/zmqutil.h
120+++ b/src/zmq/zmqutil.h
121@@ -8,12 +8,13 @@
122 #include <logging.h>
123 
124 #include <string>
125+#include <string_view>
126 
127-/** Log ZMQ error at the specified level */
128-void zmqError(BCLog::Level level, const std::string& str);
129+/** Log ZMQ failure with Error severity. Use for fatal errors that prevent ZMQ functionality. */
130+void LogZmqError(std::string_view context);
131 
132-/** Log ZMQ error at Debug level */
133-void zmqErrorDebug(const std::string& str);
134+/** Log ZMQ failure with Warning severity. Use for non-fatal issues that may impact functionality. */
135+void LogZmqWarning(std::string_view context);
136 
137 /** Prefix for unix domain socket addresses (which are local filesystem paths) */
138 const std::string ADDR_PREFIX_IPC = "ipc://"; // used by libzmq, example "ipc:///root/path/to/file"

fanquake renamed this:
~~Log ZMQ bind error at Error level, abort startup on ZMQ init error (#33715)~~
zmq: Log bind error at Error level, abort startup on init error
on Oct 31, 2025

DrahtBot added the label RPC/REST/ZMQ on Oct 31, 2025

A-Manning force-pushed on Oct 31, 2025

in test/functional/interface_zmq.py:188 in 58cfd1cfc0 outdated

184@@ -185,8 +185,7 @@ def setup_zmq_test(self, services, *, recv_timeout=60, sync_blocks=True, ipv6=Fa
185     def test_basic(self, unix = False):
186         self.log.info(f"Running basic test with {'ipc' if unix else 'tcp'} protocol")
187 
188-        # Invalid zmq arguments don't take down the node, see #17185.
189-        self.restart_node(0, ["-zmqpubrawtx=foo", "-zmqpubhashtx=bar"])
190+        self.restart_node(0, [])

stickies-v commented at 12:22 pm on October 31, 2025:

Instead of disabling the test, we should probably test that invalid arguments shut down the node, as per the new behaviour?

A-Manning commented at 4:43 pm on October 31, 2025:

Done

stickies-v commented at 12:23 pm on October 31, 2025: contributor

Concept ACK, the new shutdown behaviour seems much more sane.

A-Manning force-pushed on Oct 31, 2025

Log ZMQ bind error at Error level, abort startup on ZMQ init error

Co-authored-by: Ash Manning <10554686+A-Manning@users.noreply.github.com>

05661ef06b

A-Manning force-pushed on Oct 31, 2025

in test/functional/interface_zmq.py:190 in 05661ef06b

184@@ -185,8 +185,10 @@ def setup_zmq_test(self, services, *, recv_timeout=60, sync_blocks=True, ipv6=Fa
185     def test_basic(self, unix = False):
186         self.log.info(f"Running basic test with {'ipc' if unix else 'tcp'} protocol")
187 
188-        # Invalid zmq arguments don't take down the node, see #17185.
189-        self.restart_node(0, ["-zmqpubrawtx=foo", "-zmqpubhashtx=bar"])
190+        # Invalid zmq arguments should cause exit with an error
191+        self.stop_node(0)
192+        self.nodes[0].assert_start_raises_init_error(extra_args=["-zmqpubrawtx=foo", "-zmqpubhashtx=bar"])

pinheadmz commented at 3:46 pm on November 3, 2025:

Would be nice to assert the expected_msg here. Also, to more explicitly cover the issue in #33715 I’d expect to see a test where zmq is set to a valid tcp address but one that is already in use (you should be able to attempt a bind to node’s p2p or rpc port)

pinheadmz commented at 3:46 pm on November 3, 2025: member

concept ACK

pinheadmz commented at 3:48 pm on November 3, 2025: member

Would be nice to get review from @promag or @laanwj because of the discussion in #17185 and https://github.com/bitcoin/bitcoin/pull/17445

zmq: Log bind error at Error level, abort startup on init error #33727

Code Coverage & Benchmarks

Reviews