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

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.

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

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

Code Coverage & Benchmarks

Reviews