This adds a new RPC method getzmqnotifications, which returns information about all active ZMQ notification endpoints. This is useful for software that layers on top of bitcoind, so it can verify that ZeroMQ is enabled and also figure out where it should listen.
See #13526.
This moves the used instance of CZMQNotificationInterface from a static
variable in init.cpp to a globally-accessible one declared in
zmq/zmqnotificationinterface.h. The variable is also renamed to
g_zmq_notification_interface, to be consistent with other globals.
We need this to implement a new RPC method "getzmqnotifications" (see
https://github.com/bitcoin/bitcoin/issues/13526) in a follow up.
32@@ -29,6 +33,15 @@ CZMQNotificationInterface::~CZMQNotificationInterface()
33 }
34 }
35
36+void CZMQNotificationInterface::AddJSONNotifiers(UniValue& publish) const
37+{
38+ for (const auto* n : notifiers) {
39+ const std::string& type = n->GetType();
40+ assert(type.substr(0, 3) == "pub");
41+ publish.pushKV (type.substr(3), n->GetAddress());
32@@ -29,6 +33,15 @@ CZMQNotificationInterface::~CZMQNotificationInterface()
33 }
34 }
35
36+void CZMQNotificationInterface::AddJSONNotifiers(UniValue& publish) const
std::list<const CZMQAbstractNotifier*> and cannot return the existing notifiers. But on second thought, that seems not too bad - and decoupling from UniValue makes sense. I’ve changed that accordingly.
32@@ -29,6 +33,15 @@ CZMQNotificationInterface::~CZMQNotificationInterface()
33 }
34 }
35
36+void CZMQNotificationInterface::AddJSONNotifiers(UniValue& publish) const
37+{
38+ for (const auto* n : notifiers) {
39+ const std::string& type = n->GetType();
40+ assert(type.substr(0, 3) == "pub");
Because the current code is meant to only add types that start with "pub" - if that is not the case, it is a bug with the code (or at least means the code here has to be updated as well).
But if you prefer, we could just ignore those that don’t start with "pub" and print a warning to debug.log instead. Or we could just keep the "pub" prefix in the output - whatever you think is best.
8+from test_framework.util import *
9+
10+
11+class RPCZMQTest(BitcoinTestFramework):
12+ def set_test_params(self):
13+ self.num_nodes = 2
restart_node with the options options you want to test.
Maybe just return as follow:
0[
1 { "type": "pubhashtx", "address": "tcp://127.0.0.1:28332" }
2]
And this supports multiple notifications of the same type.
"pub" prefix - so there’s no more assert.
28@@ -29,6 +29,14 @@ CZMQNotificationInterface::~CZMQNotificationInterface()
29 }
30 }
31
32+std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotifiers() const
33+{
34+ std::list<const CZMQAbstractNotifier*> result;
return notifiers?
notifiers is a list of non-const CZMQAbstractNotifier, and I’d rather not return the mutable objects. That’s the reason why I originally had this “add to JSON” method - but I think that the additional copy done here is probably not relevant in practice.
32+ + HelpExampleCli("getzmqnotifications", "")
33+ + HelpExampleRpc("getzmqnotifications", "")
34+ );
35+
36+ UniValue publish(UniValue::VARR);
37+ if (g_zmq_notification_interface != nullptr) {
An alternative is to make these RPC available (registered) if g_zmq_notification_interface != nullptr so this check could be removed.
Not sure what others think.
I was thinking about this, and my opinion is that we should have them always available (if ZMQ is compiled in at least) - as in the current code. The reason is that if a wallet frontend or such uses this RPC, then checking whether any notification is registered at all (which is equivalent to checking for non-nullness of g_zmq_notification_interface) is part of the reason for having this RPC in the first place. And while it may then catch the “method not found” error, this makes things more complicated. Having the RPC and returning an empty response seems more useful for this case to me.
But of course I’m happy to change that if others disagree.
Consider an hypothetical zmqaddnotification, shouldn’t it raise an error if g_zmq_notification_interface == nullptr?
this makes things more complicated
Why? Also, if your “frontend” is calling these RPC then it would be bad configuration if these are not present.
But g_zmq_notification_interface being null just means that we have no notifications enabled through the command-line. So with zmqaddnotification, I would imagine that it actually is able to add a notification also in that case (in fact, that could be the primary usage - do not configure any notifications through the CLI and instead add them later by RPC). I. e., zmqaddnotification would either initialise g_zmq_notification_interface if it was null, or we would always initialise it (even if no notifications are specified) on startup.
(We then might want to have a separate argument to turn off ZMQ completely - but that would be something new and is not what we have today!)
Of course, a frontend would have to handle the case of the method failing just in case. But I think if the daemon has ZMQ support but was just started without notifications, then the method should simply return that information (empty array), rather than not be present. Especially with a hypothetical addzmqnotification in the future, this behaviour makes more sense for me.
But
g_zmq_notification_interfacebeing null just means that we have no notifications enabled through the command-line.
Indeed, forgot about that.
28@@ -29,6 +29,14 @@ CZMQNotificationInterface::~CZMQNotificationInterface()
29 }
30 }
31
32+std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotifiers() const
33+{
34+ std::list<const CZMQAbstractNotifier*> result;
35+ for (const auto* n : notifiers)
{}.
58+
59+} // anonymous namespace
60+
61+void RegisterZMQRPCCommands(CRPCTable& t)
62+{
63+ for (const auto& c : commands)
{}.
{} for the if body that throws the help message - while it seems none of these use them, they are required by the style guide there as well.
43+ publish.push_back(obj);
44+ }
45+ }
46+
47+ UniValue result(UniValue::VOBJ);
48+ result.pushKV("publish", publish);
Now that each notification has the type, could just return the array:
0[
1 { "type": "...", "address": "..." },
2 ...
3]
34+ );
35+
36+ UniValue publish(UniValue::VARR);
37+ if (g_zmq_notification_interface != nullptr) {
38+ const std::list<const CZMQAbstractNotifier*> notifiers = g_zmq_notification_interface->GetActiveNotifiers();
39+ for (const auto* n : notifiers) {
Could avoid above line:
0for (auto n : g_zmq_notification_interface->GetActiveNotifiers()) {
0@@ -0,0 +1,35 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2018 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test for the ZMQ RPC methods."""
6+
7+from test_framework.test_framework import BitcoinTestFramework
8+from test_framework.util import *
assert_equal, so this certainly makes sense).
8+from test_framework.util import *
9+
10+
11+class RPCZMQTest(BitcoinTestFramework):
12+
13+ address = "tcp://127.0.0.1:28332"
I actually like it better like this, even if it is only used in two places for now. And if we add a addzmqnotification later, it will be used in more.
But if you strongly prefer this to be inlined, I can of course do it.
This adds a new RPC method "getzmqnotifications", which returns
information about all active ZMQ notification endpoints. This is useful
for software that layers on top of bitcoind, so it can verify that
ZeroMQ is enabled and also figure out where it should listen.
See https://github.com/bitcoin/bitcoin/issues/13526.
I vaguely remember I also started work on something like this a few years ago, with a zmq refactor that was never eventually merged, but I still think it’s a good idea.
What I needed at the time was a way to ask a bitcoind instance whether it was listening on zmq, and if so, on that endpoints. This prevents having to pass in zmq configuration to the client application separately as well.
Tested ACK caac39b0ace38aa088d88c1a5a9a9dbb4d2e893f.
15+ def set_test_params(self):
16+ self.num_nodes = 1
17+ self.setup_clean_chain = True
18+
19+ def run_test(self):
20+ self._test_getzmqnotifications()
interface_zmq.py. Otherwise it will fail.