RPC: Add new "getzmqnotifications" method #13570

domob1812 · 2018-06-29T14:17:00Z

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 bitcoin#13526) in a follow up.

domob1812 · 2018-06-29T14:17:51Z

I've created two commits, because the first is a trivial and "unrelated" refactor - I think it makes sense for the review and version history to have it separate. But I'm happy to squash them if you think that's better.

Empact · 2018-06-29T16:42:36Z

src/zmq/zmqnotificationinterface.cpp

Whitespace - maybe run clang-format.

Good catch, fixed.

promag

Concept ACK. Please update code format.

promag · 2018-06-29T23:36:55Z

src/zmq/zmqnotificationinterface.cpp

IMO should not use UniValue here. Return a list instead?

I thought about this - returning a list means (because I think we should not return a list of non-const notifiers) that we have to create a new 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.

promag · 2018-06-29T23:37:44Z

src/zmq/zmqnotificationinterface.cpp

Why assert?

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.

promag

Maybe just return as follow:

[
    { "type": "pubhashtx", "address": "tcp://127.0.0.1:28332" }
]

And this supports multiple notifications of the same type.

promag · 2018-06-30T09:26:19Z

test/functional/rpc_zmq.py

Use 1 node and below restart_node with the options options you want to test.

Done - besides reducing the number of nodes, this also puts the args right next to the corresponding test; so indeed a good suggestion!

DrahtBot · 2018-06-30T11:35:59Z

Note to reviewers: This pull request conflicts with the following ones:

rpc: Rename size to vsize in mempool related calls #13008 (rpc: Rename size to vsize in mempool related calls by IPGlider)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

domob1812 · 2018-06-30T14:07:24Z

The latest update also changed the format as suggested to an array. We now also return just the literal type without removing the "pub" prefix - so there's no more assert.

promag · 2018-07-01T15:05:57Z

src/zmq/zmqnotificationinterface.cpp

Could just return notifiers?

No because 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.

promag · 2018-07-01T15:11:50Z

src/zmq/zmqrpc.cpp

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_interface being null just means that we have no notifications enabled through the command-line.

Indeed, forgot about that.

promag · 2018-07-04T20:51:11Z

src/zmq/zmqnotificationinterface.cpp

Missing {}.

Done. I've seen single-line blocks without it (even in this file), but the style guide is indeed pretty clear that this should be avoided going forward.

promag · 2018-07-04T20:51:31Z

src/zmq/zmqrpc.cpp

Missing {}.

Done. I've also added {} 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.

promag · 2018-07-04T20:53:42Z

src/zmq/zmqrpc.cpp

Now that each notification has the type, could just return the array:

[ { "type": "...", "address": "..." }, ... ]

Makes sense, changed.

promag · 2018-07-04T20:55:49Z

src/zmq/zmqrpc.cpp

Could avoid above line:

for (auto n : g_zmq_notification_interface->GetActiveNotifiers()) {

promag · 2018-07-04T20:56:36Z

test/functional/rpc_zmq.py

Could specify import symbols.

Done (only assert_equal, so this certainly makes sense).

promag · 2018-07-04T20:58:03Z

test/functional/rpc_zmq.py

nit, inline this (only 2 times)?

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.

No strong opinion.

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 bitcoin#13526.

laanwj · 2018-07-05T15:41:49Z

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 caac39b.

161e8d4 RPC: Add new getzmqnotifications method. (Daniel Kraft) caac39b Make ZMQ notification interface instance global. (Daniel Kraft) Pull request description: 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. Tree-SHA512: edce722925741c84ddbf7b3a879fc9db1907e5269d0d97138fe724035d93ee541c2118c24fa92f4197403f380d0e25c2fda5ca6c62d526792ea749cf527a99a0

maflcko · 2018-07-11T14:39:47Z

test/functional/rpc_zmq.py

+        self.setup_clean_chain = True
+
+    def run_test(self):
+        self._test_getzmqnotifications()


The test should be skipped similar to interface_zmq.py. Otherwise it will fail.

That is indeed a good point! Since this was already merged, should I create a new PR with this fix?

I'll work on a fix and send in a PR with it as soon as I'm ready. Thanks again for spotting this!

Created #13646.

Skip the new rpc_zmq.py test if ZMQ is disabled; otherwise it fails because the RPC method getzmqnotifications is not available. This fixes an oversight in bitcoin#13570.

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 bitcoin#13526) in a follow up. Github-Pull: bitcoin#13570 Rebased-From: 2c8f16340aa8212c687aa05450077c51f968aaf9

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 bitcoin#13526. Github-Pull: bitcoin#13570 Rebased-From: 76c04a84d4e02acbd4ca78af3d4d7b9e854c1160

161e8d4 RPC: Add new getzmqnotifications method. (Daniel Kraft) caac39b Make ZMQ notification interface instance global. (Daniel Kraft) Pull request description: 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 bitcoin#13526. Tree-SHA512: edce722925741c84ddbf7b3a879fc9db1907e5269d0d97138fe724035d93ee541c2118c24fa92f4197403f380d0e25c2fda5ca6c62d526792ea749cf527a99a0

fanquake added the RPC/REST/ZMQ label Jun 29, 2018

Empact reviewed Jun 29, 2018

View reviewed changes

promag reviewed Jun 29, 2018

View reviewed changes

domob1812 force-pushed the zmq branch from dd140e3 to 25e2753 Compare June 30, 2018 07:30

promag reviewed Jun 30, 2018

View reviewed changes

domob1812 force-pushed the zmq branch from 25e2753 to fcc6529 Compare June 30, 2018 14:05

promag reviewed Jul 1, 2018

View reviewed changes

promag reviewed Jul 4, 2018

View reviewed changes

domob1812 force-pushed the zmq branch from fcc6529 to 161e8d4 Compare July 5, 2018 06:02

laanwj merged commit 161e8d4 into bitcoin:master Jul 9, 2018

domob1812 deleted the zmq branch July 9, 2018 15:46

maflcko reviewed Jul 11, 2018

View reviewed changes

domob1812 mentioned this pull request Jul 12, 2018

tests: Skip rpc_zmq.py when ZMQ is disabled #13646

Closed

Bushstar mentioned this pull request Jul 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#354

Merged

ryanofsky mentioned this pull request Aug 3, 2018

Refactor: separate wallet from node #10973

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

RPC: Add new "getzmqnotifications" method #13570

RPC: Add new "getzmqnotifications" method #13570

Uh oh!

Conversation

domob1812 commented Jun 29, 2018

Uh oh!

domob1812 commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domob1812 commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

promag left a comment •

edited

Loading

DrahtBot commented Jun 30, 2018 •

edited

Loading

laanwj commented Jul 5, 2018 •

edited

Loading