Add ZeroMQ notifications #6103

promag · 2015-05-04T13:58:31Z

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

ajweiss · 2015-05-04T16:01:02Z

src/zmqports.cpp

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


This entire file needs to be retabbed.

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

Fixed in upcoming commit

ajweiss · 2015-05-04T16:07:14Z

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

promag · 2015-05-04T16:10:19Z

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.

sipa · 2015-05-04T16:13:07Z

src/ui_interface.h

@@ -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;


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.

Fixed in upcoming commit

ajweiss · 2015-05-04T16:22:32Z

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 · 2015-05-04T18:38:38Z

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

jonasschnelli · 2015-05-05T06:57:15Z

doc/zmq.md

+Currently, the ZeroMQ facility only needs to have the ZeroMQ endpoint
+specified:
+
+    $ bitcoind -zmqpub=tcp://127.0.0.1/28332


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

fixed in upcoming commit

promag · 2015-05-05T10:29:54Z

src/zmqports.cpp

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


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

promag · 2015-05-05T10:37:13Z

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

jonasschnelli · 2015-05-05T11:28:05Z

src/zmqports.h

+// Global state
+extern bool fZMQPub;
+
+#if ENABLE_ZMQ


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.

FYI #4594 (comment)

@promag: Thanks. Yes this makes sense.

jonasschnelli · 2015-05-05T11:40:26Z

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

jonasschnelli · 2015-05-05T11:47:33Z

src/init.cpp

@@ -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"]);


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

jonasschnelli · 2015-05-05T12:03:33Z

@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 · 2015-05-05T12:07:12Z

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

jonasschnelli · 2015-05-05T12:32:00Z

src/zmqports.cpp

+    CBlock blk;
+    {
+        LOCK(cs_main);
+


nit: L128 has whitespace.

jonasschnelli · 2015-05-05T12:39:23Z

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 · 2015-05-05T13:15:20Z

@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 · 2015-05-05T13:21:06Z

@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 · 2015-05-05T13:23:46Z

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 · 2015-05-05T13:27:37Z

@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 · 2015-05-05T13:37:28Z

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

promag · 2015-09-11T00:39:36Z

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

jonasschnelli · 2015-09-11T07:03:54Z

src/zmq/zmqpublishnotifier.cpp

@@ -0,0 +1,176 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-2014 The Bitcoin Core developers


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

jonasschnelli · 2015-09-11T07:15:48Z

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 · 2015-09-11T07:20:30Z

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 · 2015-09-11T07:22:38Z

src/zmq/zmqnotificationinterface.cpp

+#include "main.h"
+#include "streams.h"
+#include "util.h"
+#include "netbase.h"


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

promag · 2015-09-11T09:08:24Z

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?

jgarzik · 2015-09-15T16:09:54Z

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 · 2015-09-15T16:16:44Z

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.

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 · 2015-09-16T10:33:03Z

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

Add ZeroMQ notifications Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6103 - bitcoin/bitcoin#6684 - bitcoin/bitcoin#6686 - bitcoin/bitcoin#6736 - bitcoin/bitcoin#6739 - bitcoin/bitcoin#6743 - bitcoin/bitcoin#6768 - bitcoin/bitcoin#6779 - bitcoin/bitcoin#6810 - bitcoin/bitcoin#6927 - bitcoin/bitcoin#6980 (only upgrading zeromq) - bitcoin/bitcoin#6680 - bitcoin/bitcoin#7058 - bitcoin/bitcoin#7621 - bitcoin/bitcoin#7335 (only parts affecting `zmq_test.py`) - bitcoin/bitcoin#7853 (only parts affecting `zmq_test.py`) - bitcoin/bitcoin#7762 - bitcoin/bitcoin#7993 (only upgrading zeromq) - bitcoin/bitcoin#8238 - bitcoin/bitcoin#8701 - bitcoin/bitcoin#6685 Closes #2020.

ajweiss reviewed May 4, 2015
View reviewed changes

sipa reviewed May 4, 2015
View reviewed changes

jonasschnelli reviewed May 5, 2015
View reviewed changes

promag force-pushed the zmq branch from cf38373 to 8e9ff5c Compare May 5, 2015 10:23

promag reviewed May 5, 2015
View reviewed changes

promag force-pushed the zmq branch from 8e9ff5c to a268d1c Compare May 5, 2015 10:33

jonasschnelli reviewed May 5, 2015
View reviewed changes

promag force-pushed the zmq branch from a268d1c to 3f4f2b0 Compare May 5, 2015 11:56

jonasschnelli reviewed May 5, 2015
View reviewed changes

src/zmqports.cpp

CBlock blk;

{

LOCK(cs_main);

Copy link

Contributor

jonasschnelli May 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: L128 has whitespace.

promag force-pushed the zmq branch from 3f4f2b0 to 4470354 Compare May 5, 2015 13:29

promag force-pushed the zmq branch 2 times, most recently from 929ba9b to 7565cf2 Compare May 5, 2015 13:46

jonasschnelli reviewed Sep 11, 2015
View reviewed changes

promag force-pushed the zmq branch from 65b5df5 to 0197327 Compare September 11, 2015 07:12

jonasschnelli reviewed Sep 11, 2015
View reviewed changes

promag force-pushed the zmq branch 2 times, most recently from 53cf8e4 to 1e48d06 Compare September 11, 2015 15:05

João Barbosa and others added 4 commits September 16, 2015 10:59

Add UpdatedBlockTip signal to CMainSignals and CValidationInterface

5624e05

Depends: Add ZeroMQ package

1136879

Add ZeroMQ support. Notify blocks and transactions via ZeroMQ

e6a14b6

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

QA: Add ZeroMQ RPC test

029e278

promag force-pushed the zmq branch from 1e48d06 to 029e278 Compare September 16, 2015 10:01

jgarzik merged commit 029e278 into bitcoin:master Sep 16, 2015

jgarzik pushed a commit that referenced this pull request Sep 16, 2015

Merge pull request #6103

13b8282

jgarzik mentioned this pull request Sep 16, 2015

ZMQ fails to static link on Windows #6681

Closed

promag deleted the zmq branch September 16, 2015 21:54

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Oct 15, 2015

zeromq: Update to final PR bitcoin#6103 merged upstream

9190892

maflcko mentioned this pull request Oct 16, 2015

clang-format: Recently added files #6839

Closed

str4d mentioned this pull request Jan 31, 2017

Add ZeroMQ notifications zcash/zcash#2050

Merged

promag mentioned this pull request Nov 6, 2017

ZMQ: Add decodedtx topic for JSON tx publishing #11614

Closed

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

		@@ -0,0 +1,176 @@
		// Copyright (c) 2009-2010 Satoshi Nakamoto
		// Copyright (c) 2009-2014 The Bitcoin Core developers

Add ZeroMQ notifications #6103

Add ZeroMQ notifications #6103

Uh oh!

Conversation

promag commented May 4, 2015

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!

ajweiss commented May 4, 2015

Uh oh!

promag commented May 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajweiss commented May 4, 2015

Uh oh!

jonasschnelli commented May 4, 2015

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 commented May 5, 2015

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!

jonasschnelli commented May 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 5, 2015

Uh oh!

jonasschnelli commented May 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 5, 2015

Uh oh!

promag commented May 5, 2015

Uh oh!

jonasschnelli commented May 5, 2015

Uh oh!

sipa commented May 5, 2015

Uh oh!

promag commented May 5, 2015

Uh oh!

promag commented May 5, 2015

Uh oh!

promag commented Sep 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Sep 11, 2015

Uh oh!

jonasschnelli commented Sep 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 11, 2015

Uh oh!

jgarzik commented Sep 15, 2015

Uh oh!

jonasschnelli commented Sep 15, 2015

Uh oh!

promag commented Sep 16, 2015

Uh oh!

Uh oh!