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

Add ZeroMQ notifications #6103

Merged
merged 4 commits into from Sep 16, 2015
Merged

Add ZeroMQ notifications #6103

merged 4 commits into from Sep 16, 2015

Conversation

@promag
Copy link
Member

promag commented May 4, 2015

Continues Johnathan Corgan and Jeff Garzik's work. Supercedes #4594 and #5303.
As discussed in #6072 and #5328.


static void zmqError(const char *str)
{
LogPrint("ZMQ error: %s, errno=%s\n", str, zmq_strerror(errno));

This comment has been minimized.

Copy link
@ajweiss

ajweiss May 4, 2015

Contributor

This entire file needs to be retabbed.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

L39: s/LogPrint/LogPrintf otherwise you start introducing a debug category with string identifier "ZMQ error: %s, errno=%s\n".

This comment has been minimized.

Copy link
@promag

promag May 5, 2015

Author Member

Fixed in upcoming commit

@ajweiss
Copy link
Contributor

ajweiss commented May 4, 2015

Are you planning on supplying an RPC/regression test for this?

@promag
Copy link
Member Author

promag commented May 4, 2015

That work was in previous PR. I can't see a use case where this is useful. Maybe @jmcorgan have something to say about that, otherwise I would remove this RPC call.

@@ -95,6 +97,12 @@ class CClientUIInterface

/** New block has been accepted */
boost::signals2::signal<void (const uint256& hash)> NotifyBlockTip;

/** New block has been accepted */
boost::signals2::signal<void (const CBlock& block)> NotifyAcceptBlock;

This comment has been minimized.

Copy link
@sipa

sipa May 4, 2015

Member

As mentioned in one of the previous PRs, block acceptance is really not interesting or useful. It does not mean the block is part of the best chain, or even that it is valid. Use the NotifyBlockTip signal place, which indicates a new tip of the best chain was accepted.

This comment has been minimized.

Copy link
@promag

promag May 5, 2015

Author Member

Fixed in upcoming commit

@ajweiss
Copy link
Contributor

ajweiss commented May 4, 2015

As far as I know, no RPC tests for ZMQ support have yet been PR'd.

I think there might be some confusion here. When I say RPC tests, I mean the Python regression tests that live qa/rpc-tests tree. They're pretty much only called RPC tests because of history and the primary way they interface with bitcoind. They exercise and verify many large bits of functionality and are always useful! Case in point, when REST support was originally added, it had some pretty serious bugs that weren't caught because there was no RPC test. See PR #5379 for an example of how RPC tests for the REST interface were implemented.

I suppose RPC tests could be PR'd separately, but I bet this will go in faster and with more enthusiasm if tests are added.

@jonasschnelli
Copy link
Member

jonasschnelli commented May 4, 2015

@promag Nice that you have started recycle/update/rebase this!
I have plans to test this during this week and therefore would be willing to write some RPC'like tests.

doc/zmq.md Outdated
Currently, the ZeroMQ facility only needs to have the ZeroMQ endpoint
specified:

$ bitcoind -zmqpub=tcp://127.0.0.1/28332

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

this should be: bitcoind -zmqpub=tcp://127.0.0.1:28332 (dash colon instead slash before port)
Edit: dash / colon

This comment has been minimized.

Copy link
@promag

promag May 5, 2015

Author Member

fixed in upcoming commit

}

uiInterface.NotifyAcceptBlock.connect(ZMQPublishBlock);
uiInterface.NotifyRelayTx.connect(ZMQPublishTransaction);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

Does it make sense to use the uiInterface CClientUIInterface as signaling layer for zmq? Or would it be better to introduce a custom signaling class/struct?

This comment has been minimized.

Copy link
@promag

promag May 5, 2015

Author Member

I believe this discussion should happen in a different PR.

@@ -282,6 +282,9 @@ static const CRPCCommand vRPCCommands[] =
{ "network", "getpeerinfo", &getpeerinfo, true },
{ "network", "ping", &ping, true },

/* ZMQ notification */
{ "zmq", "getzmqurl", &getzmqurl, true },

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

What's the use case of getzmqurl? Why should someone need to get the ZMQ URL over RPC?

This comment has been minimized.

Copy link
@promag

promag May 5, 2015

Author Member

Removed in upcoming commit

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

I didn't say it has to be removed. I just don't see a usecase for this. Maybe @jgarzik can roll this up?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

Or maybe something for @jmcorgan


// Serialize block
CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
ss.reserve(1000000); // FIXME use defined constant

This comment has been minimized.

This comment has been minimized.

Copy link
@promag

promag May 5, 2015

Author Member

I've changed this to use zeromq multipart messages. Changed in upcoming commit

@promag promag force-pushed the uphold:zmq branch from cf38373 to 8e9ff5c May 5, 2015
LOCK(cs_main);

CBlockIndex* pblockindex = mapBlockIndex[hash];
if(!ReadBlockFromDisk(blk, pblockindex))

This comment has been minimized.

Copy link
@promag

promag May 5, 2015

Author Member

I believe this is the right way of getting a block.

@promag promag force-pushed the uphold:zmq branch from 8e9ff5c to a268d1c May 5, 2015
@promag
Copy link
Member Author

promag commented May 5, 2015

I'm planning to add the option -zmqformat=raw|json. This way a subscriber doesn't need to rely on 3rd libs to process the messages. What do you think?
Pending on #6108

// Global state
extern bool fZMQPub;

#if ENABLE_ZMQ

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

Is the reason why the #ifdefs are here instead of wrapping the code-part in init.cpp to lower the #ifdef in init.cpp? Maybe also something for @jgarzik to answer.

This comment has been minimized.

Copy link
@promag

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

@promag: Thanks. Yes this makes sense.

@jonasschnelli
Copy link
Member

jonasschnelli commented May 5, 2015

Would be nice if you could cherry-pick / pull-in this RPC/ZMQ test case:
jonasschnelli@91334e7

@@ -1022,6 +1029,11 @@ bool AppInit2(boost::thread_group& threadGroup)
BOOST_FOREACH(string strDest, mapMultiArgs["-seednode"])
AddOneShot(strDest);

if (mapArgs.count("-zmqpub"))
{
ZMQInitialize(mapArgs["zmqpub"]);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

typo: should be ZMQInitialize(mapArgs["-zmqpub"]); (mind the dash)

@promag promag force-pushed the uphold:zmq branch from a268d1c to 3f4f2b0 May 5, 2015
@jonasschnelli
Copy link
Member

jonasschnelli commented May 5, 2015

@promag: I would not add JSON support and keep ZMQ as clean and low-level as possible. Keeping it bin only can make things faster and with JSON there is always a risks of things get handled different during enc-/decoding of JSON streams depending on the library you use.

@jonasschnelli
Copy link
Member

jonasschnelli commented May 5, 2015

Tested reviewed leak-checked ACK.
nits: usage of uiInterface as "signal layer"

CBlock blk;
{
LOCK(cs_main);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli May 5, 2015

Member

nit: L128 has whitespace.

@jonasschnelli
Copy link
Member

jonasschnelli commented May 5, 2015

nit:
signaling disconnect is missing

diff --git a/src/zmqports.cpp b/src/zmqports.cpp
index d083292..bd1229c 100644
--- a/src/zmqports.cpp
+++ b/src/zmqports.cpp
@@ -155,6 +155,9 @@ void ZMQShutdown()

         zmq_ctx_destroy(zmqContext);
         zmqContext = 0;
+
+        uiInterface.NotifyBlockTip.disconnect(ZMQPublishBlock);
+        uiInterface.NotifyRelayTx.disconnect(ZMQPublishTransaction);
     }

     fZMQPub = false;
@promag
Copy link
Member Author

promag commented May 5, 2015

@jonasschnelli Regarding the output formats, I don't think having JSON is that bad, it is just an option.
Actually I think we could support 3 formats for now:

  • network: currently supported
  • json: needs BlockToUni to convert to JSON (similar to RPC getblock)
  • hash: just send the block hash or txid (this allows a subscriber to queue the notification to process it later)
@jonasschnelli
Copy link
Member

jonasschnelli commented May 5, 2015

@promag I agree with 1 (p2p) and 3 (hash). IMO the ZMQ interface is a push/notify interface (for now). If someone like to get JSON output he could do a RPC request with received data. Adding JSON format over ZMQ level would just encourage user "to go the wrong way".
Adding a way of only receiving hashes over ZMQ would be good.
But i'm ready to hear other opinions.

@sipa
Copy link
Member

sipa commented May 5, 2015

Specifically for blocks, I'm not sure that pushing full blocks is even useful, unless we make it push all new blocks (including intermediary reorged ones). The interesting information is "there is a new block chain tip, and this is it".

@promag
Copy link
Member Author

promag commented May 5, 2015

@sipa consider the following use case: a subscriber wants to know if there are transactions (in the new block chain tip) with outputs targeting a set of addresses.

@promag promag force-pushed the uphold:zmq branch from 3f4f2b0 to 4470354 May 5, 2015
@promag
Copy link
Member Author

promag commented May 5, 2015

I'm not sure if publishing a block of 20MB is problematic.

@promag promag force-pushed the uphold:zmq branch 2 times, most recently from 929ba9b to 7565cf2 May 5, 2015
@promag promag force-pushed the uphold:zmq branch Sep 10, 2015
@promag
Copy link
Member Author

promag commented Sep 11, 2015

@jonasschnelli @jgarzik @jtimon please review the documentation change.

@jonasschnelli
jonasschnelli reviewed Sep 11, 2015
View changes
src/zmq/zmqpublishnotifier.cpp Outdated
@@ -0,0 +1,176 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2014 The Bitcoin Core developers

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Sep 11, 2015

Member

nit: remove L1 AND s/2009-2014/2015?

@promag promag force-pushed the uphold:zmq branch Sep 11, 2015
@jonasschnelli
Copy link
Member

jonasschnelli commented Sep 11, 2015

Just git a build error on debian:

./configure --enable-debug --with-gui=no
[...]
checking whether to build ZMQ support... yes
checking for ZMQ... yes
[...]
make -j5
[...]
  CXX      wallet/libbitcoin_wallet_a-walletdb.o
zmq/zmqpublishnotifier.cpp: In function ‘int zmq_send_multipart(void*, const void*, size_t, ...)’:
zmq/zmqpublishnotifier.cpp:36:61: error: ‘zmq_msg_send’ was not declared in this scope
         rc = zmq_msg_send(&msg, sock, data ? ZMQ_SNDMORE : 0);
[...]
zmq/zmqnotificationinterface.cpp:118:33: error: ‘zmq_ctx_destroy’ was not declared in this scope
         zmq_ctx_destroy(pcontext);                                                          ^

The source of the problem is probably an outdated libzmq. I have installed libzmq1(-dev):
i A libzmq1 - lightweight messaging kernel (shared library)

During configure we might ask for zmq_msg_send to detect if the library is available (or similar)?

@jonasschnelli
Copy link
Member

jonasschnelli commented Sep 11, 2015

Using libzmq3 solved the problem. But IIRC, Debian 7 (wheezy) does not support libzmq3 out of the standard repository. A tiny check during ./configure would be nice.

@jonasschnelli
jonasschnelli reviewed Sep 11, 2015
View changes
src/zmq/zmqnotificationinterface.cpp Outdated
#include "main.h"
#include "streams.h"
#include "util.h"
#include "netbase.h"

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Sep 11, 2015

Member

nit: i think this include can be removed. Compiles fine on OSX/Debian without this include.

@jonasschnelli
jonasschnelli reviewed Sep 11, 2015
View changes
src/zmq/zmqpublishnotifier.cpp Outdated

#include "zmqpublishnotifier.h"
#include "main.h"
#include "net.h"

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Sep 11, 2015

Member

nit: I think this include can be removed. Compiles fine on OSX/Debian without this include.

This comment has been minimized.

Copy link
@jtimon

jtimon Sep 11, 2015

Member

Are you sure this is not because net.h is included in main.h? The same goes for the other removed include [ie netbase.h is included in net.h iirc].

This comment has been minimized.

Copy link
@sipa

sipa via email Sep 11, 2015

Member
@promag
Copy link
Member Author

promag commented Sep 11, 2015

Using libzmq3 solved the problem. But IIRC, Debian 7 (wheezy) does not support libzmq3 out of the standard repository. A tiny check during ./configure would be nice.

@theuni can you take a look?

@promag promag force-pushed the uphold:zmq branch 2 times, most recently Sep 11, 2015
@jgarzik
Copy link
Contributor

jgarzik commented Sep 15, 2015

In general, this appears merge-ready once

  • travis is happy
  • the @sipa lock comment is addressed.

We can iterate further once it's in-tree.

@jonasschnelli
Copy link
Member

jonasschnelli commented Sep 15, 2015

Agree with @jgarzik. There are serval proposal for improvements (autodetect libzmq3, avoid locking by passing CBlockIndex*, minor cleanups), but shouldn't hold back the merge.
The only thing that might should be looked at is the travis issue: https://travis-ci.org/bitcoin/bitcoin/jobs/79866093. Maybe its not related to this PR. Maybe a rebase&force-push solves that issue.

theuni and others added 4 commits May 6, 2015
Continues Johnathan Corgan's work.
Publishing multipart messages

Bugfix: Add missing zmq header includes

Bugfix: Adjust build system to link ZeroMQ code for Qt binaries
@promag promag force-pushed the uphold:zmq branch to 029e278 Sep 16, 2015
@promag
Copy link
Member Author

promag commented Sep 16, 2015

@jonasschnelli rebase@force-push solved.
@laanwj @sipa I believe it's all in place to merge.

@jgarzik jgarzik merged commit 029e278 into bitcoin:master Sep 16, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jgarzik pushed a commit that referenced this pull request Sep 16, 2015
Jeff Garzik
@promag promag deleted the uphold:zmq branch Sep 16, 2015
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Oct 15, 2015
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.