Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC: Add new "getzmqnotifications" method #13570

Merged
merged 2 commits into from Jul 9, 2018

Conversation

Projects
None yet
7 participants
@domob1812
Copy link
Contributor

domob1812 commented Jun 29, 2018

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.

Make ZMQ notification interface instance global.
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
#13526) in a follow up.
@domob1812

This comment has been minimized.

Copy link
Contributor Author

domob1812 commented Jun 29, 2018

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.

src/zmq/zmqnotificationinterface.cpp Outdated
for (const auto* n : notifiers) {
const std::string& type = n->GetType();
assert(type.substr(0, 3) == "pub");
publish.pushKV (type.substr(3), n->GetAddress());

This comment has been minimized.

@Empact

Empact Jun 29, 2018

Member

Whitespace - maybe run clang-format.

This comment has been minimized.

@domob1812

domob1812 Jun 30, 2018

Author Contributor

Good catch, fixed.

@promag
Copy link
Member

promag left a comment

Concept ACK. Please update code format.

src/zmq/zmqnotificationinterface.cpp Outdated
@@ -29,6 +33,15 @@ CZMQNotificationInterface::~CZMQNotificationInterface()
}
}

void CZMQNotificationInterface::AddJSONNotifiers(UniValue& publish) const

This comment has been minimized.

@promag

promag Jun 29, 2018

Member

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

This comment has been minimized.

@domob1812

domob1812 Jun 30, 2018

Author Contributor

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.

src/zmq/zmqnotificationinterface.cpp Outdated
{
for (const auto* n : notifiers) {
const std::string& type = n->GetType();
assert(type.substr(0, 3) == "pub");

This comment has been minimized.

@promag

promag Jun 29, 2018

Member

Why assert?

This comment has been minimized.

@domob1812

domob1812 Jun 30, 2018

Author Contributor

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.

@domob1812 domob1812 force-pushed the domob1812:zmq branch Jun 30, 2018

@promag
Copy link
Member

promag left a comment

Maybe just return as follow:

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

And this supports multiple notifications of the same type.

test/functional/rpc_zmq.py Outdated

class RPCZMQTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 2

This comment has been minimized.

@promag

promag Jun 30, 2018

Member

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

This comment has been minimized.

@domob1812

domob1812 Jun 30, 2018

Author Contributor

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jun 30, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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 domob1812 force-pushed the domob1812:zmq branch Jun 30, 2018

@domob1812

This comment has been minimized.

Copy link
Contributor Author

domob1812 commented Jun 30, 2018

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.

@@ -29,6 +29,14 @@ CZMQNotificationInterface::~CZMQNotificationInterface()
}
}

std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotifiers() const
{
std::list<const CZMQAbstractNotifier*> result;

This comment has been minimized.

@promag

promag Jul 1, 2018

Member

Could just return notifiers?

This comment has been minimized.

@domob1812

domob1812 Jul 2, 2018

Author Contributor

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.

src/zmq/zmqrpc.cpp Outdated
);

UniValue publish(UniValue::VARR);
if (g_zmq_notification_interface != nullptr) {

This comment has been minimized.

@promag

promag Jul 1, 2018

Member

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.

This comment has been minimized.

@domob1812

domob1812 Jul 2, 2018

Author Contributor

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.

This comment has been minimized.

@promag

promag Jul 2, 2018

Member

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.

This comment has been minimized.

@domob1812

domob1812 Jul 2, 2018

Author Contributor

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.

This comment has been minimized.

@promag

promag Jul 2, 2018

Member

But g_zmq_notification_interface being null just means that we have no notifications enabled through the command-line.

Indeed, forgot about that.

src/zmq/zmqnotificationinterface.cpp Outdated
std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotifiers() const
{
std::list<const CZMQAbstractNotifier*> result;
for (const auto* n : notifiers)

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

Missing {}.

This comment has been minimized.

@domob1812

domob1812 Jul 5, 2018

Author Contributor

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.

src/zmq/zmqrpc.cpp Outdated

void RegisterZMQRPCCommands(CRPCTable& t)
{
for (const auto& c : commands)

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

Missing {}.

This comment has been minimized.

@domob1812

domob1812 Jul 5, 2018

Author Contributor

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.

src/zmq/zmqrpc.cpp Outdated
}

UniValue result(UniValue::VOBJ);
result.pushKV("publish", publish);

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

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

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

This comment has been minimized.

@domob1812

domob1812 Jul 5, 2018

Author Contributor

Makes sense, changed.

src/zmq/zmqrpc.cpp Outdated
UniValue publish(UniValue::VARR);
if (g_zmq_notification_interface != nullptr) {
const std::list<const CZMQAbstractNotifier*> notifiers = g_zmq_notification_interface->GetActiveNotifiers();
for (const auto* n : notifiers) {

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

Could avoid above line:

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

This comment has been minimized.

@domob1812

domob1812 Jul 5, 2018

Author Contributor

Done.

test/functional/rpc_zmq.py Outdated
"""Test for the ZMQ RPC methods."""

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import *

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

Could specify import symbols.

This comment has been minimized.

@domob1812

domob1812 Jul 5, 2018

Author Contributor

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


class RPCZMQTest(BitcoinTestFramework):

address = "tcp://127.0.0.1:28332"

This comment has been minimized.

@promag

promag Jul 4, 2018

Member

nit, inline this (only 2 times)?

This comment has been minimized.

@domob1812

domob1812 Jul 5, 2018

Author Contributor

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 comment has been minimized.

@promag

promag Jul 5, 2018

Member

No strong opinion.

RPC: Add new getzmqnotifications method.
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.

@domob1812 domob1812 force-pushed the domob1812:zmq branch to 161e8d4 Jul 5, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 5, 2018

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.

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

laanwj added a commit that referenced this pull request Jul 9, 2018

Merge #13570: RPC: Add new "getzmqnotifications" method
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

@domob1812 domob1812 deleted the domob1812:zmq branch Jul 9, 2018

self.setup_clean_chain = True

def run_test(self):
self._test_getzmqnotifications()

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 11, 2018

Member

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

This comment has been minimized.

@domob1812

domob1812 Jul 11, 2018

Author Contributor

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

This comment has been minimized.

@domob1812

domob1812 Jul 12, 2018

Author Contributor

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

This comment has been minimized.

@domob1812

domob1812 Jul 12, 2018

Author Contributor

Created #13646.

domob1812 added a commit to domob1812/bitcoin that referenced this pull request Jul 12, 2018

tests: Skip rpc_zmq.py when ZMQ is disabled.
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.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jul 30, 2018

Make ZMQ notification interface instance global.
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

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jul 30, 2018

RPC: Add new getzmqnotifications method.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.