Add ZeroMQ notifications

promag commented at 1:58 pm on May 4, 2015: member

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

in src/zmqports.cpp: in 77ecbe3e01 outdated

34+static void ZMQPublishBlock(const CBlock &blk);
35+static void ZMQPublishTransaction(const CTransaction &tx);
36+
37+static void zmqError(const char *str)
38+{
39+  LogPrint("ZMQ error: %s, errno=%s\n", str, zmq_strerror(errno));

ajweiss commented at 4:01 pm on May 4, 2015:

This entire file needs to be retabbed.

jonasschnelli commented at 6:52 am on May 5, 2015:

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

promag commented at 9:56 am on May 5, 2015:

Fixed in upcoming commit

ajweiss commented at 4:07 pm on May 4, 2015: contributor

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

promag commented at 4:10 pm on May 4, 2015: member

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.

in src/ui_interface.h: in 77ecbe3e01 outdated

 96@@ -95,6 +97,12 @@ class CClientUIInterface
 97 
 98     /** New block has been accepted */
 99     boost::signals2::signal<void (const uint256& hash)> NotifyBlockTip;
100+
101+    /** New block has been accepted */
102+    boost::signals2::signal<void (const CBlock& block)> NotifyAcceptBlock;

sipa commented at 4:13 pm on May 4, 2015:

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.

promag commented at 9:56 am on May 5, 2015:

Fixed in upcoming commit

ajweiss commented at 4:22 pm on May 4, 2015: contributor

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 commented at 6:38 pm on May 4, 2015: contributor

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

in doc/zmq.md: in 77ecbe3e01 outdated

60+## Configuration
61+
62+Currently, the ZeroMQ facility only needs to have the ZeroMQ endpoint
63+specified:
64+
65+    $ bitcoind -zmqpub=tcp://127.0.0.1/28332

jonasschnelli commented at 6:57 am on May 5, 2015:

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

promag commented at 9:53 am on May 5, 2015:

fixed in upcoming commit

in src/zmqports.cpp: in 77ecbe3e01 outdated

61+    zmqPubSocket = 0;
62+    return;
63+  }
64+
65+  uiInterface.NotifyAcceptBlock.connect(ZMQPublishBlock);
66+  uiInterface.NotifyRelayTx.connect(ZMQPublishTransaction);

jonasschnelli commented at 7:00 am on May 5, 2015:

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?

promag commented at 9:55 am on May 5, 2015:

I believe this discussion should happen in a different PR.

in src/rpcserver.cpp: in 77ecbe3e01 outdated

281@@ -282,6 +282,9 @@ static const CRPCCommand vRPCCommands[] =
282     { "network",            "getpeerinfo",            &getpeerinfo,            true  },
283     { "network",            "ping",                   &ping,                   true  },
284 
285+    /* ZMQ notification */
286+    { "zmq",                "getzmqurl",              &getzmqurl,              true  },

jonasschnelli commented at 7:27 am on May 5, 2015:

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

promag commented at 9:55 am on May 5, 2015:

Removed in upcoming commit

jonasschnelli commented at 11:21 am on May 5, 2015:

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

jonasschnelli commented at 11:42 am on May 5, 2015:

Or maybe something for @jmcorgan

in src/zmqports.cpp: in 77ecbe3e01 outdated

122+  if (!zmqPubSocket)
123+    return;
124+
125+  // Serialize block
126+  CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
127+  ss.reserve(1000000); // FIXME use defined constant

jonasschnelli commented at 7:59 am on May 5, 2015:

I think this line can be removed. Also L112. See also: https://github.com/bitcoin/bitcoin/blob/master/src/rpcblockchain.cpp#L308 and https://github.com/bitcoin/bitcoin/blob/master/src/rest.cpp#L182

promag commented at 9:54 am on May 5, 2015:

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

promag force-pushed on May 5, 2015

in src/zmqports.cpp: in 8e9ff5c88d outdated

125+    CBlock blk;
126+    {
127+        LOCK(cs_main);
128+    
129+        CBlockIndex* pblockindex = mapBlockIndex[hash];
130+        if(!ReadBlockFromDisk(blk, pblockindex))

promag commented at 10:29 am on May 5, 2015:

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

promag force-pushed on May 5, 2015

promag commented at 10:37 am on May 5, 2015: member

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

in src/zmqports.h: in a268d1cfc0 outdated

13+#include <string>
14+
15+// Global state
16+extern bool fZMQPub;
17+
18+#if ENABLE_ZMQ

jonasschnelli commented at 11:28 am on May 5, 2015:

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.

promag commented at 11:37 am on May 5, 2015:

FYI #4594 (comment)

jonasschnelli commented at 11:42 am on May 5, 2015:

@promag: Thanks. Yes this makes sense.

jonasschnelli commented at 11:40 am on May 5, 2015: contributor

Would be nice if you could cherry-pick / pull-in this RPC/ZMQ test case: https://github.com/jonasschnelli/bitcoin/commit/91334e70ee5b2713ebbb7d5dbd6a5a29936aba66

in src/init.cpp: in a268d1cfc0 outdated

1028@@ -1022,6 +1029,11 @@ bool AppInit2(boost::thread_group& threadGroup)
1029     BOOST_FOREACH(string strDest, mapMultiArgs["-seednode"])
1030         AddOneShot(strDest);
1031 
1032+    if (mapArgs.count("-zmqpub"))
1033+    {
1034+        ZMQInitialize(mapArgs["zmqpub"]);

jonasschnelli commented at 11:47 am on May 5, 2015:

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

promag force-pushed on May 5, 2015

jonasschnelli commented at 12:03 pm on May 5, 2015: contributor

@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 commented at 12:07 pm on May 5, 2015: contributor

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

in src/zmqports.cpp: in 3f4f2b0986 outdated

123+        return;
124+
125+    CBlock blk;
126+    {
127+        LOCK(cs_main);
128+

jonasschnelli commented at 12:32 pm on May 5, 2015:

nit: L128 has whitespace.

jonasschnelli commented at 12:39 pm on May 5, 2015: contributor

nit: signaling disconnect is missing

 0diff --git a/src/zmqports.cpp b/src/zmqports.cpp
 1index d083292..bd1229c 100644
 2--- a/src/zmqports.cpp
 3+++ b/src/zmqports.cpp
 4@@ -155,6 +155,9 @@ void ZMQShutdown()
 5
 6         zmq_ctx_destroy(zmqContext);
 7         zmqContext = 0;
 8+
 9+        uiInterface.NotifyBlockTip.disconnect(ZMQPublishBlock);
10+        uiInterface.NotifyRelayTx.disconnect(ZMQPublishTransaction);
11     }
12
13     fZMQPub = false;

promag commented at 1:15 pm on May 5, 2015: member

@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 commented at 1:21 pm on May 5, 2015: contributor

@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 commented at 1:23 pm on May 5, 2015: member

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 commented at 1:27 pm on May 5, 2015: member

@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 force-pushed on May 5, 2015

promag commented at 1:37 pm on May 5, 2015: member

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

promag force-pushed on May 5, 2015

sipa commented at 1:52 pm on May 5, 2015: member

That use case would not be covered, as the current code only reports the new tip, not potential intermediate blocks that were connected as part of a reorganization.

A possibility is reporting all new chain blocks, but what if this (due to a fork bug, perhaps) ends up reporting 100s of megabytes?

promag commented at 1:54 pm on May 5, 2015: member

@sipa right, so publishing the new tip hash and having the consumer walk the block chain would be more appropriate/secure.

jonasschnelli commented at 1:59 pm on May 5, 2015: contributor

For now i see the ZMQ extension as a notification-only one-way interface. I guess it’s more optimized if a user gets informed about a new block hash (or new mempool tx hash) and then he decides if he likes to load the block with or without tx details over REST or RPC.

promag commented at 2:04 pm on May 5, 2015: member

Agree on network binary format and hash format? Other publish strategies can be discussed later after this PR.

jonasschnelli commented at 2:05 pm on May 5, 2015: contributor

@promag: i think you need to get rid of commit 454285518915d73e55674cf702bd0372a2da266e

promag force-pushed on May 5, 2015

promag commented at 2:31 pm on May 5, 2015: member

@jonasschnelli rebased missing updated doc and test to reflect multipart message

promag commented at 2:39 pm on May 5, 2015: member

Should we use CValidationInterface instead of CClientUIInterface?

sipa commented at 2:45 pm on May 5, 2015: member

Yes.

promag force-pushed on May 5, 2015

promag commented at 3:57 pm on May 5, 2015: member

Pending questions:

support network binary format and hash format?
class name CZMQPublisher : public CValidationInterface?
use CValidationInterface::SetBestChain instead of CClientUIInterface::NotifyBlockTip?

sipa commented at 4:00 pm on May 5, 2015: member

Hash is enough for me. Binary makes sense if we make sure the full set of newly accepted blocks is announced.
Sounds good.
SetBestChain won’t work, as it is only called after the best chain on disk is changed (which is infrequently). I’ll write a patch to add the signals you need to CValidationInterface.

promag commented at 4:03 pm on May 5, 2015: member

@sipa At the moment CValidationInterface is very related to CWallet, but it shouldn’t right?

sipa commented at 4:03 pm on May 5, 2015: member

@promag It used to be called CWalletInterface. We renamed it because it shouldn’t be used exclusively for wallets.

laanwj added the label Feature on May 5, 2015

in src/zmqports.h: in 12cefe533e outdated

24+    void Initialize(const std::string &endpoint, Format format);
25+    void Shutdown();
26+
27+protected: // CValidationInterface
28+    void SyncTransaction(const CTransaction &tx, const CBlock *pblock);
29+    void UpdatedBlockTip(const CBlock &block);

promag commented at 5:12 pm on May 5, 2015:

waiting @sipa patch, for now assuming UpdatedBlockTip exists in CValidationInterface.

in src/zmqports.h: in 12cefe533e outdated

13+{
14+public:
15+    enum Format
16+    {
17+        HashFormat,
18+        NetworkFormat

promag commented at 5:13 pm on May 5, 2015:

Not sure if this is OK or NETWORK_FORMAT. Same for HashFormat.

in src/zmqports.cpp: in 12cefe533e outdated

166+        uint256 hash = block.GetHash();
167+
168+        // Send multipart message
169+        int rc = Publish("BLK", 3, ZMQ_SNDMORE);
170+        if (rc==0)
171+            Publish(hash.begin(), hash.size(), 0);

promag commented at 5:13 pm on May 5, 2015:

@jonasschnelli is this OK?

jonasschnelli commented at 6:41 pm on May 5, 2015:

LGTM.

promag force-pushed on May 5, 2015

promag commented at 5:44 pm on May 5, 2015: member

Later will address #5303#discussion-diff-26264618

jonasschnelli commented at 6:42 pm on May 5, 2015: contributor

Will do additional tests when UpdatedBlockTip is ready.

promag commented at 9:29 pm on May 5, 2015: member

It seems Travis is not building with zeromq. Can this be addressed?

theuni commented at 0:33 am on May 6, 2015: member

@promag Yes. You’ll need to add the dependency. You can pull this commit: https://github.com/theuni/bitcoin/commit/3661302abfca852a871b8b3cb406d5d10dfc33ae to get travis to test. Edit: Updated version: https://github.com/theuni/bitcoin/commit/eda02cf35626a6bc988aa9bd6df217df2a23c7ae

Note, however, that the windows build currently fails because zeromq refuses to static-link with mingw. I chose to leave this broken rather than disabling the windows build because we’ll have to come up with a plan, even if it’s just to explicitly disable it there.

promag commented at 9:09 am on May 6, 2015: member

@sipa if you like I can add the new signal to CValidationInterface. I suppose I can emit it before the NotifyBlockTip.

promag commented at 10:07 am on May 6, 2015: member

@theuni BTW why static linking? and why not dynamic linking on travis because of http://docs.travis-ci.com/user/ci-environment/#Messaging-Technology

theuni commented at 5:41 pm on May 6, 2015: member

@promag Travis builds exactly as we build for release. The current build failure means that we wouldn’t be able to package zeromq into a windows binary. Until that’s fixed, failing is the right thing to do imo. The question is: is it worth it to try to work around their mingw situation, or just disable it for windows.

jonasschnelli commented at 6:47 am on May 7, 2015: contributor

Having ZMQ for windows as well would be nice in near future. With ZMQ we could place a (daemon)script into the /contrib folder and deprecate the -notify features. IIRC there where serval issues with the -notify commands on windows. @promag: I think it would be nice if you could complete this and try to add the new signal to CValidationInterface()

promag commented at 2:44 pm on May 18, 2015: member

@theuni should we disable zmq in windows? ping @sipa

promag force-pushed on May 18, 2015

in src/init.cpp: in ac4f864bab outdated

356@@ -345,6 +357,10 @@ std::string HelpMessage(HelpMessageMode mode)
357                     
358 #endif
359 
360+    strUsage += HelpMessageGroup(_("ZeroMQ publisher options"));
361+    strUsage += HelpMessageOpt("-zmqpub=<endpoint>",_("Publish blocks and transactions on 'endpoint'"));
362+    strUsage += HelpMessageOpt("-zmqformat=hash|network>",_("Publish format is either hash (default) or network"));

jonasschnelli commented at 3:44 pm on May 19, 2015:

s/-zmqformat=hash|network**>/-zmqformat=<hash|network>**/

promag force-pushed on May 20, 2015

promag commented at 3:41 pm on May 20, 2015: member

Disabled zeromq in windows because zmq fails to build as a static lib in mingw.

promag force-pushed on May 20, 2015

jtimon commented at 6:32 am on May 22, 2015: contributor

utACK

luke-jr commented at 8:03 pm on May 23, 2015: member

Why not publish both formats and let clients choose which to subscribe to?

theuni commented at 9:41 pm on May 23, 2015: member

@promag That only disables the windows build for travis, not for anyone actually building for windows.

Please instead use something like:

0packages:=boost openssl
1packages_darwin:=zeromq
2packages_linux:=zeromq

Please also document the fact that it’s unusable for Windows due to upstream build issues.

promag commented at 10:13 pm on May 23, 2015: member

@luke-jr you mean one address per format? @theuni right, will do.

luke-jr commented at 11:26 pm on May 23, 2015: member

@promag I mean using ZeroMQ’s topics so clients can filter which they want.

promag commented at 8:52 am on May 25, 2015: member

@luke-jr Actually I was thinking removing the option to publish the entire transaction/block. The block will have a maximum size of 20mb. And writing all formats to the socket sequentially can cause some lag each time there is a new block. Also, at the moment, writing to the socket happens in the main thread, and the publisher should be as fast as possible.

At the moment we use the hash format, and the hash is enqueued in a persistent/reliable queue so that it can be processed later on a easy pace. Then the RPC interface is used to get more details about it. We think the load of bitcoind is much lower than publishing the entire data.

promag force-pushed on May 25, 2015

jtimon commented at 4:01 pm on May 28, 2015: contributor

In fact, apart from several formats and topics for PUB, we could allow the option of using either PUB or PUSH instead of forcing it to be PUB? PUSH should be much faster than PUB, for big things like blocks (both with 1 MB and 20 MB blocks, which is something completely orthogonal to this PR).

EDIT: I think the option for a topic could be useful for smaller things like transactions, for example, and there’s no reason not to make it generic just because that option may be slow for blocks. In fact, since you should be able to configure each event independently, so I think we should have a block publisher/pusher option and an analogous one for transactions.

This way you could, for example, configure a topic-based publisher for transactions and a simple pusher for blocks (or the opposite, push both: any combination).

promag commented at 9:01 am on May 29, 2015: member

@jtimon IMHO this notification mechanism should be as simple as possible. Just publishing the event/hash is enough for the downstream to make RPC calls to get more information about blocks/transactions, including computed data which is not in the raw data, for instance, confirmations etc.

In our use case we tried to avoid RPC calls upstream, but ended giving up because we didn’t want to miss the blockchain maintenance of bitcoind, ie, handling forks, double spends, etc. So, with this notifications we just avoid notify-* scripts and RPC polling.

Moreover, I would never send the raw data, because it is too much and requires the downstream to “know” how to parse the raw data. Handling JSON RPC is much easier.

Regarding the socket type, I wouldn’t choose a socket where the send operation is blocking as this could lead to, either internal queue in a separate thread, or making bitcoind malfunction. Therefore, IIRC PUB/SUB is the only type that has non-blocking send.

promag force-pushed on May 29, 2015

jtimon commented at 10:06 am on May 29, 2015: contributor

@jtimon IMHO this notification mechanism should be as simple as possible.

IMO it should be as flexible as possible. That’s what I love most about zmq, anyway.

In our use case we tried to avoid RPC calls upstream, but ended giving up because we didn’t want to miss the blockchain maintenance of bitcoind, ie, handling forks, double spends, etc. So, with this notifications we just avoid notify-* scripts and RPC polling.

That’s seems good for your use case. Other uses of zmq could be to precisely replace rpc calls (although we’re not supporting REQ/REP in this PR and that’s fine). Remember that RPCs also take locks and all that. Anyway, please don’t assume that everybody wants to use the zmq integration in the same way you do.

Moreover, I would never send the raw data, because it is too much and requires the downstream to “know” how to parse the raw data. Handling JSON RPC is much easier.

Zmq also supports json, but I can see some potential consumers preferring raw data.

Regarding the socket type, I wouldn’t choose a socket where the send operation is blocking as this could lead to, either internal queue in a separate thread, or making bitcoind malfunction. Therefore, IIRC PUB/SUB is the only type that has non-blocking send.

I didn’t know that PULL/PUSH was blocking, but doesn’t make much sense to me. Then why wouldn’t people just use a PUB/SUB with a single subscriber that then pushes to another sockets (to avoid making the original send blocking)? Can you link me to the zmq documentation that says PUSH sends are blocking but PUB are not? Even if that’s the case, it may be worth it for some use cases.

Anyway, your arguments for not supporting topics in PUB/SUB (as Luke-Jr proposes) seem invalid to me, specially if you were assuming that only hashes will be sent.

promag commented at 10:25 am on May 29, 2015: member

@jtimon From http://api.zeromq.org/4-0:zmq-socket

When a ZMQ_PUSH socket enters the mute state due to having reached the high water mark for all downstream nodes, or if there are no downstream nodes at all, then any zmq_send(3) operations on the socket shall block until the mute state ends or at least one downstream node becomes available for sending; messages are not discarded.

promag commented at 10:45 am on May 29, 2015: member

IMHO the true purpose of this PR is to eventually avoid/deprecate notify scripts. It should provide the CValidationInterface signals to the outside world. I agree with all that flexibility/options but for me it is out of scope of this PR.

jtimon commented at 11:22 am on May 29, 2015: contributor

Oh I see, of course, push is blocking when you don’t have ready pullers…that has a simple solution: have pullers. That’s a problem for the user to solve. IMO not a good reason not to support PUSH ( which is so similar to PUB in all other things).

About the scope of this PR, sure, this doesn’t have to do everything, but for example supporting PUB/SUB topics definitely seems within scope.

If we’re going to leave pushers for later, that’s fine, sunce we want a different option to activate it anyway. Still, the current options could indicate that this is for PUB explicitly, for the options to be more clear when/if we add PUSH (or even REP) later. Also, one should be able to configure the transactions and locks events independently.

jtimon commented at 11:26 am on May 29, 2015: contributor

So in summary, I would separate zmqpub into zmqpubtx and zmqpubblock or something similar, and I would add support for topics, everything else can be left for later.

promag commented at 11:49 am on May 29, 2015: member

What you mean by zmqpubtx and zmqpubblock? Two pub sockets binded to different addresses?

jmcorgan commented at 4:24 pm on May 29, 2015: contributor

On Fri, May 29, 2015 at 2:02 AM, João Barbosa notifications@github.com wrote:

Regarding the socket type, I wouldn’t choose a socket where the send operation is blocking as this could lead to, either internal queue in a separate thread, or making bitcoind malfunction. Therefore, IIRC PUB/SUB is the only type that has non-blocking send.

This is precisely the reason PUB was chosen when I originally implemented this.

Johnathan Corgan Corgan Labs - SDR Training and Development Services Intro to SDR Classes - June 4-5, Columbia MD, June 29-30, El Segundo, CA http://corganlabs.com

ajweiss commented at 7:27 pm on May 29, 2015: contributor

This pull already implements topics via construction of multipart messages. They are “BLK” and “TXN”. If the zmq library on the publisher side is recent enough, it will filter on the publisher side as per the subscriber’s topic preferences.

This, however, doesn’t help all that much as ideally you’d want something that can tell whether or not anyone is subscribed to a topic so you can save the cost of serialization when it’s not necessary. AFAIK, there’s no simple way to ask zmq if anyone is subscribed or what topics are subscribed.

Whether or not the cost of serialization/zmq_sending on all transactions and blocks is enough to justify adding configuration options (and translation/documentation overhead) to selectively turn block or transaction publishing on/off is an open question. I have not measured it.

jtimon commented at 6:23 am on June 1, 2015: contributor

What you mean by zmqpubtx and zmqpubblock? Two pub sockets binded to different addresses?

Yes. well in fact maybe you want to connect both to the same socket, but yes the goal would be that you are able to configure them independently (maybe you confiure blocks but not transactions or viceversa).

luke-jr commented at 3:21 am on June 2, 2015: member

If the goal is to merely notify of hashes, I suggest an external program triggered by -*notify may be better.

promag commented at 9:59 am on June 2, 2015: member

@luke-jr notify scripts only support wallet transactions and blocks. There is no support for mempool notifications (#6072).

Beside that, executing a script for each hash is overkill, specially when lots of transactions change state like when the blockchain is incremented. Also, notify scripts execute in the same machine the bitcoind runs whereas sockets provide much more flexibility.

jtimon commented at 10:37 am on June 2, 2015: contributor

So does it make sense to be able to configure independent sockets for transactions and blocks? You could still connect both to the same port instead of binding them, presumably used in combination of a zmq Forwarder or binding a single subscriber.

luke-jr commented at 10:49 am on June 2, 2015: member

@promag Ok, you’ve made your point. I suggest at least offering RPC over ZMQ though, but that’s I guess out of scope here.

jmcorgan commented at 1:37 pm on June 2, 2015: contributor

RPC over ZMQ is indeed out of scope on this pull request, but definitely worth considering.

promag commented at 4:27 pm on June 3, 2015: member

Agree on discarding RPC for now.

So I think we could allow arguments -zmq{type}{format}{object}={address} where

type = pub (push in the future)
format = hash | raw
object = transaction | block
address = a valid zmq socket address

Example: -zmqpubhashblock=ipc:///tmp/bitcoin.block.hash -zmqpubrawtransaction=ipc:///tmp/bitcoin.transaction.raw

jtimon commented at 4:51 pm on June 3, 2015: contributor

Agree on discarding RPC for now.

So I think we could allow arguments -zmq{type}{format}{object}={address} where

type = pub (push in the future) format = hash | raw object = transaction | block address = a valid zmq socket address

Sounds great to me.

promag commented at 5:27 pm on June 3, 2015: member

Ops, forgot to add the topic option, which is important if an address is shared. Unless topic is the same as the object.

promag force-pushed on Jun 5, 2015

promag commented at 5:28 pm on June 5, 2015: member

@jonasschnelli @jgarzik please review the refactor, still wip

in src/init.cpp: in df435c5fc3 outdated

360@@ -349,9 +361,12 @@ std::string HelpMessage(HelpMessageMode mode)
361     strUsage += HelpMessageOpt("-walletnotify=<cmd>", _("Execute command when a wallet transaction changes (%s in cmd is replaced by TxID)"));
362     strUsage += HelpMessageOpt("-zapwallettxes=<mode>", _("Delete all wallet transactions and only recover those parts of the blockchain through -rescan on startup") +
363         " " + _("(1 = keep tx meta data e.g. account owner and payment request information, 2 = drop tx meta data)"));
364-                    
365+
366 #endif
367 
368+    strUsage += HelpMessageGroup(_("ZeroMQ publisher options"));
369+    strUsage += HelpMessageOpt("-zmq<type><format><object>=<address>",_("Enable notification through zmq socket <address> (type=pub, format=hash|raw, object=block|transaction)"));

jonasschnelli commented at 7:58 pm on June 5, 2015:

It looks like that this is not implemented like that. Example: zmqpubrawblock is not supported. Only zmqpubhashblock and zmqpubhashtransaction is supported IMO. At least the should be a warning or error if you try to add a zmq channel which is not supported.

promag commented at 8:35 pm on June 5, 2015:

As stated it’s just an initial version to ask for comments. The usage help will havê the supported options.

jonasschnelli commented at 8:09 pm on June 5, 2015: contributor

This PR needs some love.

documentation out of date (or lets say out of sync because it’s not yet available)
./contrib/zmq/zmq_sub.py out of date, doesn’t run.
./contrib/zmq/zmq_sub.py as no +x (executable) flag like linearize.py, etc.
zmq rpc test does not run, out of date
the factory thing as well as the flexible -zmq<pub><...> feels over the top and should throw an error if given “format” could not generate a listener.

I really think ZMQ within bitcoin-core would be a great step, but this PR lacks here and there at the moment.

promag commented at 8:40 pm on June 5, 2015: member

Agree but before polishing and squash I want to be sure that the refactor is acceptable.

Instead of having lots of switch/ifs I added a notifier type for each type of notification. Later other types of notifications can be added easilly. If it’s OK then I’ll update everything.

jonasschnelli commented at 8:10 am on June 6, 2015: contributor

Another thing: If i add -zmqpubhashblock=tcp://127.0.0.1:28332 -zmqpubhashtransaction=tcp://127.0.0.1:2833 (both notification types) it won’t send blockhashes through zmq (didn’t analyze in detail).

Conceptual: I’m not sure if the flexible cmd arg is the right thing. It is like settings config values within a config arg key which feels somehow strange. Also unsure why lots of things are compiled in when --enable-zmq=no (cmd args get parsed and factory gets created even when having --enable-zmq=no during compile time. This feels wrong).

Feel free to pull in the zmq test / zmq example tool fixes: https://github.com/jonasschnelli/bitcoin/commit/fe2e286856eb35e0614fa5e1da757ff44ba7eeb1

promag commented at 9:31 am on June 8, 2015: member

@jonasschnelli Flexible command args is just a way to explain it here because of the multiple combinations. At then end the usage help will have listed just the notifiers supported. So, for instance, if command argument -zmqpubhashblock is defined then it will enable publishing hash blocks.

I’ll follow #4594 (comment) to avoid compiling zmq notifications when --enable-zmq=no.

I’ll rebase your commit and wrap everything.

promag force-pushed on Jun 8, 2015

jonasschnelli commented at 7:37 pm on June 11, 2015: contributor

Needs rebase.

promag force-pushed on Jun 12, 2015

in contrib/zmq/zmq_sub.py: in feb2534c5e outdated

0@@ -0,0 +1,38 @@
1+#!/usr/bin/env python2
2+
3+import array
4+import binascii
5+import zmq
6+
7+port = 28332
8+port = 4849

wozz commented at 12:45 pm on June 12, 2015:

extra port

in doc/zmq.md: in feb2534c5e outdated

135+
136+    except KeyboardInterrupt:
137+        zmqContext.destroy()
138+
139+
140+This example is provided in the contrib/zmq directory of the source

wozz commented at 12:45 pm on June 12, 2015:

this is out of sync

promag force-pushed on Jun 12, 2015

promag commented at 1:33 pm on June 12, 2015: member

@jonasschnelli @wozz done

nunofgs commented at 5:25 pm on June 16, 2015: none

@jtimon, @sipa, @jonasschnelli: is there a timeframe for merging this? Would love to see this in 0.12!

rebroad commented at 6:56 pm on June 16, 2015: contributor

I don’t see that anyone has asked this, and it doesn’t seem to be mentioned, but: who would use this? Is it a majority or a small minority that would? If the latter, can this functionality be added with some sort of 3rd party utility instead? If so, then I don’t think it ought to be included in Bitcoin Core as it would add bloat and reduce security.

jonasschnelli commented at 7:04 pm on June 16, 2015: contributor

@rebroad: I think consensus is that ZMQ (0mq) is desirable within bitcoin core (https://github.com/bitcoin/bitcoin/pull/2415, #5303) . At the moment, bitcoin-core users (including merchants, etc.) need to use -blocknotify and co. to get informed when a new block is available.

ZMQ would potential also allow to separate the wallet from the core and let them run in different processes.

promag commented at 10:11 am on June 17, 2015: member

@rebroad If you follow the discussion, and related pull requests, you will find enough arguments to deprecate notify scripts and have a non-intrusive notification interface.

jtimon commented at 4:51 pm on June 17, 2015: contributor

@nunofgs I don’t know, whenever is merged? I already ut ACKedm I had some nits that go resolved… re ut ACK.

in doc/zmq.md: in 47a4cb2882 outdated

14+limited service to notify external software of events like the arrival
15+of new blocks or transactions.
16+
17+The ZeroMQ facility implements a notification interface through a
18+set of specific notifiers. Currently there are notifiers that publish
19+blocks and transactions. The  supports a set of no binds a "publish" port that broadcasts all newly

jonasschnelli commented at 6:39 pm on June 18, 2015:

Linebreak is wrong.

in doc/zmq.md: in 47a4cb2882 outdated

 96+Client side, then, the ZeroMQ subscriber socket must have the
 97+ZMQ_SUBSCRIBE option set to one or either of these prefixes; without
 98+doing so will result in no messages arriving.
 99+
100+Here is a small example, in the Python language, using the
101+*python-zmq* wrapper:

jonasschnelli commented at 6:43 pm on June 18, 2015:

IMO better mention contrib/zmq/zmq_sub.py here. Otherwise we have to maintain two python examples.

promag commented at 0:49 am on June 19, 2015:

@jonasschnelli I had the same feeling, will do it.

jonasschnelli commented at 7:22 pm on June 18, 2015: contributor

Hmm.. can’t compile this PR.

OSX:

0  "CZMQNotificationInterface::Initialize()", referenced from:
1      AppInit2(boost::thread_group&, CScheduler&) in libbitcoin_server.a(libbitcoin_server_a-init.o)
2  "CZMQNotificationInterface::CreateWithArguments(std::__1::map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > const&)", referenced from:
3      AppInit2(boost::thread_group&, CScheduler&) in libbitcoin_server.a(libbitcoin_server_a-init.o)
4  "CZMQNotificationInterface::Shutdown()", referenced from:
5      Shutdown() in libbitcoin_server.a(libbitcoin_server_a-init.o)

linux:

0zmq/zmqpublishnotifier.cpp: In function ‘int zmq_send_multipart(void*, const void*, size_t, ...)’:
1zmq/zmqpublishnotifier.cpp:30:61: error: ‘zmq_msg_send’ was not declared in this scope
2zmq/zmqnotificationinterface.cpp: In member function ‘void CZMQNotificationInterface::Shutdown()’:
3zmq/zmqnotificationinterface.cpp:119:33: error: ‘zmq_ctx_destroy’ was not declared in this scope

promag commented at 0:48 am on June 19, 2015: member

@D9B4BEF9 what’s the platform and zmq version? @jonasschnelli what’s the zmq version?

jonasschnelli commented at 11:57 am on June 19, 2015: contributor

@promag: linux: libzmq1 2.2.0+dfsg-2 (debian) osx: zeromq/4.0.5_2

luke-jr commented at 7:07 am on June 24, 2015: member

zmqnotificationinterface.cpp and zmqpublishnotifier.cpp are missing #include <zmq.h>

zmqpublishnotifier.cpp is missing copyright headers

luke-jr commented at 3:35 am on July 3, 2015: member

Please cherry-pick and squash 5b2845feabe01f5b671c87bdb909b5b05202d068 and 52396aa65b9e2da7f82e5fd12d87b7a614493ca0

Then you just need to make sure copyright headers are on all new files.

promag force-pushed on Jul 9, 2015

promag commented at 8:59 am on July 9, 2015: member

@luke-jr done, thanks!

promag renamed this:
~~Add ZMQ support. Report blocks and transactions via ZMQ.~~
Add ZeroMQ notifications
on Jul 9, 2015

D9B4BEF9 commented at 5:00 am on July 17, 2015: none

@jonasschnelli A clean build environment seems to have sorted out my problem of bitcoind just pegging a core, don’t know what was going on there.

in src/init.cpp: in a3ae0c1f25 outdated

30@@ -31,7 +31,9 @@
31 #include "wallet/wallet.h"
32 #include "wallet/walletdb.h"
33 #endif
34-
35+#if ENABLE_ZMQ

Diapolo commented at 7:33 am on July 17, 2015:

Nit: IMHO the ZMQ header stuff should go below std headers and below boost, openssl etc (header group ordering). And I’m talking about all includes, not just this one.

in src/init.cpp: in a3ae0c1f25 outdated

196@@ -191,6 +197,17 @@ void Shutdown()
197     if (pwalletMain)
198         pwalletMain->Flush(true);
199 #endif
200+
201+#if ENABLE_ZMQ
202+    if (pzmqNotificationInterface)

Diapolo commented at 7:35 am on July 17, 2015:

Nit: if (pzmqNotificationInterface) { as sometime in the future this will be the default code formatting (clang cleanup).

in src/init.cpp: in a3ae0c1f25 outdated

370@@ -354,6 +371,14 @@ std::string HelpMessage(HelpMessageMode mode)
371         " " + _("(1 = keep tx meta data e.g. account owner and payment request information, 2 = drop tx meta data)"));
372 #endif
373 
374+#if ENABLE_ZMQ
375+    strUsage += HelpMessageGroup(_("ZeroMQ notification options"));

Diapolo commented at 7:36 am on July 17, 2015:

Nit: Misses a : at the end.

in src/zmq/zmqconfig.h: in a3ae0c1f25 outdated

0@@ -0,0 +1,30 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 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+
6+#ifndef BITCOIN_ZMQGLOBAL_H

Diapolo commented at 7:38 am on July 17, 2015:

This needs to be BITCOIN_ZMQ_ZMQCONFIG_H also don’t forget the header end comment.

in src/zmq/zmqconfig.h: in a3ae0c1f25 outdated

16+
17+#include <stdarg.h>
18+
19+void zmqError(const char *str);
20+
21+#if defined(HAVE_CONFIG_H)

Diapolo commented at 7:38 am on July 17, 2015:

This is duplicated and also the includes look weird, perhaps some merge-conflict-leftover?

in src/zmq/zmqabstractnotifier.h: in a3ae0c1f25 outdated

0@@ -0,0 +1,43 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 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+
6+#ifndef BITCOIN_ZMQABSTRACTNOTIFIER_H

Diapolo commented at 7:39 am on July 17, 2015:

Needs to be BITCOIN_ZMQ_ZMQABSTRACTNOTIFIER_H also as header end comment.

in src/zmq/zmqnotificationinterface.cpp: in a3ae0c1f25 outdated

10+#include "main.h"
11+#include "streams.h"
12+#include "util.h"
13+#include "netbase.h"
14+
15+#include <zmq.h>

Diapolo commented at 7:40 am on July 17, 2015:

Is this needed, seems it’s coming from the custom zmq headers.

in src/zmq/zmqpublishnotifier.cpp: in a3ae0c1f25 outdated

79+        mapPublishNotifiers.insert(std::make_pair(address, this));
80+        return true;
81+    }
82+    else
83+    {
84+        LogPrint("zmq", "  Reuse socket for address %s\n", address.c_str());

Diapolo commented at 7:42 am on July 17, 2015:

Unneeded .c_str() and there are some more in here.

in src/zmq/zmqpublishnotifier.h: in a3ae0c1f25 outdated

0@@ -0,0 +1,42 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 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+
6+#ifndef BITCOIN_ZMQPUBLISHNOTIFIER_H

Diapolo commented at 7:44 am on July 17, 2015:

Needs to be BITCOIN_ZMQ_ZMQPUBLISHNOTIFIER_H also at the header end comment.

promag commented at 9:05 pm on July 20, 2015: member

@Diapolo thanks, fixed.

promag force-pushed on Jul 21, 2015

jgarzik commented at 7:35 pm on July 23, 2015: contributor

“it works” lightly tested ACK. Needs additional testing to make sure it builds/works on a few platforms.

promag force-pushed on Jul 25, 2015

promag commented at 8:13 am on July 25, 2015: member

@venzen Please update and try again.

venzen commented at 4:21 am on July 29, 2015: none

@promag thanks, now compiles and my SUB receives blocks and transactions. However, after an initial one minute spurt of PUBs, Core apparently stops sending getdata requests (peer connection activity continues but no new blocks or txns arrive), and an RPC getrawmempool shows the mempool is now empty. bitcoind process doesn’t TERM and has to be KILLed .

If bitcoind is started without specifying any zmq PUB socket, it works as expected.

Still trying to figure out why. Confirmed for combinations of mainnet & testnet, ipv4 & onion, pruned & full-node. Tried libzmq1, libzmq3. Compiled and tested with libboost-1.55.0 in Ubuntu 14.04 on two separate hosts.

venzen commented at 6:59 am on July 30, 2015: none

@promag OK, I’ve found the reason why I was getting the behavior mentioned above. It has to do with which notifiers one specifies on the commandline, and also on the combination of notifiers. Some combinations work, others don’t. There are similar errors for different combinations.

Firstly, these combinations work as expected:

hashblock + hashtx
rawblock + rawtx
rawblock + hashtx
hashblock + rawtx

Specifying one notifier only, has the following results:

hashtx - inventory exchange stops after Connect postprocess and Connect block shows in debug.log, p2p connections, version and verack continues but no getdata requests are made.
rawtx - same as above
hashblock - node starts, creates the PUB socket but then infintely (and rapidly) repeats Publish hash block for the same block.
rawblock - same as above but more slowly

I think it is useful and resource efficient to be able to specify a single notifier only.

Combinations:

rawblock + rawtx + hashtx - inventory exchange stops after Connect postprocess and Connect block shows in debug.log, p2p connections, version and verack continues.
rawblock + rawtx + hashblock - works until hashblock is PUBd and then infinitely repeats Publish hash block for the same block.
hashblock + hashtx + rawtx - same as above
hashblock + hashtx + rawblock - works until a hashblock is PUBd, then infintely PUBs both hashblock and rawblock, e.g.:

2015-07-30 05:04:03 Publish hash block 000000000000000014ea2a49bc2165bbc9351adeb9e26870d8ba9049325fe506 2015-07-30 05:04:03 Publish raw block 000000000000000014ea2a49bc2165bbc9351adeb9e26870d8ba9049325fe506 (repeats same two messages infinitely)

Multiple identical notifier instances (this is interesting but probably irrelevant):

hashblock + 2x hashtx - works
rawtx + 2x hashblock - works
rawbloc + 2x hashtx - hashtxs are PUBd, rawblock socket is created but no rawblocks PUBd
hashblock + 2x rawtx - works

promag commented at 9:04 am on July 30, 2015: member

@venzen Thanks for the detailed report. Will look into it.

promag commented at 10:18 am on August 25, 2015: member

@venzen can you specify the zmq addresses for each case? Were you using the same address for multiple topics? tpc or ipc?

in src/zmq/zmqnotificationinterface.cpp: in 1a08136b58 outdated

122+    }
123+}
124+
125+void CZMQNotificationInterface::UpdatedBlockTip(const uint256 &hash)
126+{
127+    for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); ++i)

promag commented at 11:07 am on August 25, 2015:

Found bug here: 3rd statement ++i shouldn’t be there. @venzen I believe this is related to you problem, because with only one publisher the loop doesn’t ends.

promag force-pushed on Aug 25, 2015

promag commented at 11:25 am on August 25, 2015: member

@venzen Fixed and rebased. Tested your combinations and it’s working. Please test again.

promag commented at 10:53 pm on August 26, 2015: member

@jonasschnelli @jgarzik @luke-jr @theuni @sipa do you want to review/test?

jtimon commented at 0:09 am on August 27, 2015: contributor

Untested re-ACK. I’m looking forward to this being finally merged.

in configure.ac: in 75bf9d0ca4 outdated

136@@ -137,6 +137,12 @@ AC_ARG_ENABLE([glibc-back-compat],
137   [use_glibc_compat=$enableval],
138   [use_glibc_compat=no])
139 
140+AC_ARG_ENABLE([zmq],
141+  [AC_HELP_STRING([--disable-zmq],

jonasschnelli commented at 8:09 am on August 27, 2015:

I think this should be --enable-zqm (@theuni?)

jonasschnelli commented at 6:35 am on September 11, 2015:

nit: still think we we should use --enable-zmq with a default yes. This would conform with --enable-wallet, etc.

jgarzik commented at 4:06 pm on September 15, 2015:

Agree with the nit (though it’s not a merge blocker)

promag commented at 10:15 am on August 27, 2015: member

@jonasschnelli by default if libzmq is found then it will enabled zmq unless it’s disabled on configure. That was discussed: #4594 (comment).

in src/main.cpp: in 75bf9d0ca4 outdated

2302@@ -2303,6 +2303,8 @@ bool ActivateBestChain(CValidationState &state, const CBlock *pblock) {
2303                         pnode->PushInventory(CInv(MSG_BLOCK, hashNewTip));
2304             }
2305             // Notify external listeners about the new tip.
2306+

jonasschnelli commented at 2:39 pm on August 27, 2015:

nit: Remove this line?

in src/zmq/zmqnotificationinterface.cpp: in 75bf9d0ca4 outdated

127+    for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); )
128+    {
129+        CZMQAbstractNotifier *notifier = *i;
130+        if (notifier->NotifyBlock(hash))
131+        {
132+            i ++;

jonasschnelli commented at 2:40 pm on August 27, 2015:

nit: s/i ++/i++?

in contrib/zmq/zmq_sub.py: in 75bf9d0ca4 outdated

19+        msg = zmqSubSocket.recv_multipart()
20+        topic = str(msg[0])
21+        body = msg[1]
22+
23+        if topic == "hashblock":
24+            print "- HASH BLOK -"

jonasschnelli commented at 2:51 pm on August 27, 2015:

nit: HASH BLOK (missing C)

promag force-pushed on Aug 27, 2015

promag commented at 3:29 pm on August 27, 2015: member

@jonasschnelli fixed and rebased.

jonasschnelli commented at 3:36 pm on August 27, 2015: contributor

Did some testing.

RPC Test is broken, please cherry pick: https://github.com/jonasschnelli/bitcoin/commit/4e1859a0a55291e67bb8acf3028039668ecfd32b

RPC test sometimes fail (not always). When calling generate(10) sometimes it will result in: (11 transactions but only 9 blocks)

 0hashtx
 1hashtx
 2hashtx
 3hashblock
 4hashtx
 5hashblock
 6hashtx
 7hashblock
 8hashtx
 9hashblock
10hashtx
11hashblock
12hashtx
13hashblock
14hashtx
15hashblock
16hashtx
17hashblock
18hashtx
19hashblock

dcousens commented at 1:13 am on August 28, 2015: contributor

concept ACK

promag commented at 2:26 pm on August 28, 2015: member

@jonasschnelli thanks! However the problem is in the test, see https://github.com/jonasschnelli/bitcoin/commit/4e1859a0a55291e67bb8acf3028039668ecfd32b#commitcomment-12948902. Fixed your commit and eventually will squash. Please check again.

in qa/pull-tester/rpc-tests.sh: in a99a117ebd outdated

31@@ -32,6 +32,7 @@ testScripts=(
32     'reindex.py'
33     'decodescript.py'
34     'p2p-fullblocktest.py'
35+    'zmq_test.py'

jonasschnelli commented at 2:41 pm on August 28, 2015:

I think we need to move the zmq_test.py to the testScriptsExt list otherwise travis cannot find the zmq python module. Maybe there is also a way to make travis support python/zmq.

promag commented at 3:00 pm on August 28, 2015:

I’ll remove for now, waiting @theuni feedback.

promag force-pushed on Aug 28, 2015

promag commented at 3:23 pm on August 28, 2015: member

FYI how I run the test: python qa/rpc-tests/zmq_test.py --srcdir src/

jonasschnelli commented at 3:33 pm on August 28, 2015: contributor

@promag: Yes. Thats right. The rpc-tests.sh will get called during travis CI builds.

theuni commented at 4:15 pm on August 28, 2015: member

Travis will support it as long as it’s installed via depends. However Windows isn’t using it right now.

We’ll need to add a variable to qa/pull-tester/tests-config.sh.in similar to the ENABLE_WALLET case.

There are a few other issues I noticed in the configure.ac changes, I’ll fix those things up and provide a patch.

promag commented at 9:09 am on September 1, 2015: member

@theuni just bumping in case you forgot.

theuni commented at 3:18 am on September 2, 2015: member

@promag Thanks, indeed I did. I’ll get to this asap.

theuni commented at 9:27 pm on September 2, 2015: member

@promag it’s going to be a few days before I can get to the configure changes, but the rpc-tests should be pretty trivial:

0diff --git a/qa/pull-tester/tests-config.sh.in b/qa/pull-tester/tests-config.sh.in
1index 10f4d33..e881a95 100755
2--- a/qa/pull-tester/tests-config.sh.in
3+++ b/qa/pull-tester/tests-config.sh.in
4@@ -10,6 +10,7 @@ EXEEXT="@EXEEXT@" [@ENABLE](/bitcoin-bitcoin/contributor/enable/)_WALLET_TRUE@ENABLE_WALLET=1 [@BUILD](/bitcoin-bitcoin/contributor/build/)_BITCOIN_UTILS_TRUE@ENABLE_UTILS=1 [@BUILD](/bitcoin-bitcoin/contributor/build/)_BITCOIND_TRUE@ENABLE_BITCOIND=1
5+@ENABLE_ZMQ_TRUE@ENABLE_ZMQ=1
6
7 REAL_BITCOIND="$BUILDDIR/src/bitcoind${EXEEXT}"
8 REAL_BITCOINCLI="$BUILDDIR/src/bitcoin-cli${EXEEXT}"

Then in qa/pull-tester/rpc-tests.sh, ENABLE_ZMQ will either be empty, or set to “1” depending on whether it was enabled in configure. You can use that to conditionally add zmq_test.py to the tests array.

jtimon commented at 3:24 pm on September 4, 2015: contributor

Needs rebase (I’m asuming after #5677 ).

promag force-pushed on Sep 4, 2015

theuni commented at 4:30 pm on September 9, 2015: member

@promag looks great, assuming it works as intended

promag force-pushed on Sep 10, 2015

promag commented at 3:03 pm on September 10, 2015: member

@laanwj @sipa any chance this feature be in the next release?

in contrib/zmq/zmq_sub.py: in 56607b879e outdated

25+            print binascii.hexlify(body)
26+        elif topic == "hashtx":
27+            print '- HASH TX -'
28+            print binascii.hexlify(body)
29+        elif topic == "rawblock":
30+            print "- RAW BLOK HEADER -"

sipa commented at 3:12 pm on September 10, 2015:

s/BLOK/BLOCK/

in src/zmq/zmqpublishnotifier.cpp: in 56607b879e outdated

141+    return rc == 0;
142+}
143+
144+bool CZMQPublishRawBlockNotifier::NotifyBlock(const uint256 &hash)
145+{
146+    LogPrint("zmq", "Publish raw block %s\n", hash.GetHex());

sipa commented at 3:14 pm on September 10, 2015:

You need a cs_main lock here.

sipa commented at 3:23 pm on September 10, 2015:

Alternatively, if you send CBlockIndex* pointers in the signal rather than the hash, you don’t need to. It’s the access to mapBlockIndex that’s problematic.

jonasschnelli commented at 6:58 am on September 11, 2015:

If I’m right and understand @sipa idea correct, ReadBlockFromDisk() not requires a cs_main lock. Passing a CBlockIndex* would make CBlockIndex* pblockindex = mapBlockIndex[hash]; at L153 no longer required. The whole cs_main LOCK could then be removed.

We should definitively do this to keep bitcoin-core more responsive and lock free.

sipa commented at 3:20 pm on September 10, 2015: member

I’m not convinced that the current implementation of sending a full block (“rawblock”) makes sense.

The hashes published by UpdatedBlockTip indicate a new block tip, which in the case of a reorganization from chain A-B to A-C-D, will only notify for B and for D, but not for C. This is useful - it’s an indication that you need to go query block D, and if you don’t have its ancestor, query that too etc.

But when sending full blocks through the interface, I’m much less certain this is useful. You can’t be guaranteed to receive all blocks - only those that happen to be at the tip after every reorg/connect.

in src/zmq/zmqpublishnotifier.h: in 56607b879e outdated

0@@ -0,0 +1,42 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto

sipa commented at 3:25 pm on September 10, 2015:

This does not seem to contain any code written by Satoshi, or before 2014 :)

promag commented at 3:55 pm on September 10, 2015:

sipa commented at 4:06 pm on September 10, 2015:

Or your own name.

in src/validationinterface.cpp: in 56607b879e outdated

44@@ -43,6 +45,8 @@ void UnregisterAllValidationInterfaces() {
45     g_signals.SetBestChain.disconnect_all_slots();
46     g_signals.UpdatedTransaction.disconnect_all_slots();
47     g_signals.SyncTransaction.disconnect_all_slots();
48+    g_signals.UpdatedTransaction.disconnect_all_slots();

sipa commented at 3:26 pm on September 10, 2015:

You duplicated UpdatedTransaction here.

in doc/zmq.md: in 56607b879e outdated

15+of new blocks or transactions.
16+
17+The ZeroMQ facility implements a notification interface through a
18+set of specific notifiers. Currently there are notifiers that publish
19+blocks and transactions. The  supports a set of no binds a "publish"
20+port that broadcasts all newly arrived *and validated* blocks and

sipa commented at 3:32 pm on September 10, 2015:

This is not actually correct in the current implementation. You’d need to use CValidationInterface’s BlockChecked signal if you want that (see an example in rpcmining.cpp for submitblock).

I think it’s very important to make this distinction: what does the ZMQ block interface notify you about? Validated blocks or the accepted block chain? The first can include things like orphaned blocks, does not necessarily happen at the same time as relay, and does not tell you which block chain we’re currently considering active. The second (what is currently implemented) only notifies for changes in the tip, and may skip blocks in reorgs.

promag commented at 4:13 pm on September 10, 2015:

Will review this.

promag force-pushed on Sep 10, 2015

promag commented at 4:13 pm on September 10, 2015: member

@sipa

I’m not convinced that the current implementation of sending a full block (“rawblock”) makes sense. The hashes published by UpdatedBlockTip indicate a new block tip, which in the case of a reorganization from chain A-B to A-C-D, will only notify for B and for D, but not for C. This is useful - it’s an indication that you need to go query block D, and if you don’t have its ancestor, query that too etc.

I’m aware of that.

I think it’s very important to make this distinction: what does the ZMQ block interface notify you about? Validated blocks or the accepted block chain?

Publishing blocks happens the exact same way blocknotify script is executed.

The way we plan to use this feature is to replace notify scripts and use as a probe. The subscriber needs to determine the changes since the last notification. If he chooses to receive full block or not it will depend of his use case, after all, it’s an option.

promag force-pushed on Sep 10, 2015

promag commented at 7:44 pm on September 10, 2015: member

@sipa

Code looks good, but as mentioned elsewhere, I don’t think this is useful in its current form. If the receiver only receives the blocks that happen to be at the tip after a reorg, they need a separate mechanism to fetch the potential unannounced ancestors anyway, making the ability to receive full blocks immediately unnecessary and confusing.

The subscriber must always query from his last state, because the publisher doesn’t know the subscriber state. Suppose the subscriber is restarted, there is some system maintenance or a network issue, the publisher has no way to re-publish what the subscriber needs.

sipa commented at 7:51 pm on September 10, 2015: member

I’m just questioning what relaying full blocks adds (as opposed to just hashes), because the receiver should still have logic to fetch missing blocks in between the previous tip and the new tip.

Again: if you switch from a branch A-B-C-D for example to A-B-C’-D’-E, the current code will only publish E, and not C’ or D’. The publisher still needs to fetch E’s ancestors one by one with the current code. If the receiver is supposed to do that anyway, what does giving the full block directly add? Either give all blocks along the path, or just publish the top hash.

jtimon commented at 8:38 pm on September 10, 2015: contributor

Those 2 things are unrelated, full blocks are only allowed optionally to maintain a consistent interface with the transactions (and other types that may be supported un the future). It may be the case that nobody subscribes to raw blocks (and I believe @promag didn’t wanted to support full blocks). Take into account that we may add push/pull and/or req/rep in the future (let’s finally do this first though).

I don’t understand your point about the publisher having to fetch, as promag has explained the publisher cannot know the state of its potentially many subscribers (with potentially different states). Subscribers will need to complement with the RPC (at least until we have req/rep zmq) to get whatever else they need.

promag commented at 8:39 pm on September 10, 2015: member

+1 what @jtimon said.

sipa commented at 9:04 pm on September 10, 2015: member

I disagree. What you’re publishing is not blocks. It is updates to the chain tip. Sure, there is an actual block associated with that tip, but it’s not a natural mapping. You could as well give the chainwork of that tip, or the time.

The documentation is equally confusing: “As transactions and blocks arrive via the P2P network, Bitcoin Core will apply all the validation and standardization rules to this data, only passing along through ZeroMQ those items that pass.”. That is not what this pull request implements. It will not relay every block through ZeroMQ. It will only ever relay blocks that happen to at the tip of the chain after a reorg.

I think there are a few ways to improve this:

Only publish the chain tip updates as hashes. That’s exactly what -blocknotify does. The subscriber is expected to fetch the actual blocks whenever a notification happens.
After a reorg, publish all blocks that were activated through ZMQ. That will allow a subscriber who is already up-to-date to be able to follow new blocks as they come in and stay in sync, without needing to query RPC or REST. If you want to, I’ll implement this.
Make the block data subscriber trigger BlockChecked and not UpdatedTip. This gives the subscriber no information about what the currently active chain is, but it does give all valid blocks as soon as they are validated.

jtimon commented at 9:20 pm on September 10, 2015: contributor

Only publish the chain tip updates as hashes. That’s exactly what -blocknotify does. The subscriber is expected to fetch the actual blocks whenever a notification happens.

I believe this is the expected usage rigt now (even if it full blocks don’t make much sense with this).

After a reorg, publish all blocks that were activated through ZMQ. That will allow a subscriber who is already up-to-date to be able to follow new blocks as they come in and stay in sync, without needing to query RPC or REST. If you want to, I’ll implement this.

Make the block data subscriber trigger BlockChecked and not UpdatedTip. This gives the subscriber no information about what the currently active chain is, but it does give all valid blocks as soon as they are validated.

I’m not opposed to the other 2 solutions, I think I like the second one the most. But if it’s going to delay this. Can we at least merge a subset of this with the transactions part? That would at least introduce the zmq building dependency once and for all? I’ve been waiting for that for quite some time now…

promag commented at 9:31 pm on September 10, 2015: member

Sure, the doc needs fixing. The option -blocknotify states

0 -blocknotify=<cmd>
1       Execute command when the best block changes (%s in cmd is replaced by
2       block hash)

And I would go for something like that.

2nd is not viable, since the publisher should not assume the subscriber is up to date. The subscriber should have the option to not receive the (all supposed) missing blocks and he should have the option to defer the fetch of missing blocks. I’m fine with 3rd.

Having the full block on the notification can be helpful: a dummy subscriber, without RPC credential, could just log timestamp, blocksize, tx count for analytics.

Like @jtimon said, the ideal solution is to have REQ/REP.

sipa commented at 9:32 pm on September 10, 2015: member

I think that supporting full blocks encourages people to use it in an incorrect way, by only using those blocks and not taking reorganizations into account.

promag commented at 9:35 pm on September 10, 2015: member

How does just the hash encourages to take reorganizations into account?

sipa commented at 9:38 pm on September 10, 2015: member

Just the hash is perfectly fine. That’s what blocknotify does.

Sending the full block corresponding to the new tip encourages bad practice. People may not realize that it does not relay all blocks.

promag commented at 9:40 pm on September 10, 2015: member

I still don’t see how just the hash solves that problem. People will get the hash, query the block and then it’s the same as receiving the full block. Edit: and again, it’s just an option, and I believe we could just add remarks in the documentation.

sipa commented at 9:46 pm on September 10, 2015: member

No, they need to get the hash, query the block, see if they have the ancestor of the block, if not, query that, etc.

If you give them the blocks already, it will look like they don’t need this recursive fetch logic for ancestors. And it will work fine, except when there is a reorg, in which they only receive the new tip, and not the block before.

jgarzik commented at 9:53 pm on September 10, 2015: contributor

In general, agree w/ @sipa, just the hash on the first iteration.

Longer term, it is forseeable that some folks looking for low latency may prefer full blocks versus added round-trips. Such solutions would have a fast path - zero round trip - and a slower path fallback in case the full block was insufficient, and further block-locator style queries are needed.

promag commented at 9:54 pm on September 10, 2015: member

So it’s the same: a client gets the full block, check if the parent is there, if not retrieve and repeat.

promag commented at 9:56 pm on September 10, 2015: member

@jgarzik also agree. I just don’t agree that having just the hash option leads the subscriber to take reorgs into account. Edit: with just the hash, the subscriber could as well retrieve the tip and miss some blocks without recognising it.

jtimon commented at 9:57 pm on September 10, 2015: contributor

Yes, I fail to see the difference as well.

sipa commented at 10:01 pm on September 10, 2015: member

@promag If implemented correctly, there is no difference. You can present it as an optimization: “This informs you about the new chain tip, and as courtesy, we already give you the last block immediately so you don’t need to fetch it”. But I think the current interface gives the impression that it will relay all validated blocks, which is wrong and leads to incorrect understanding. The documentation even says so literally.

in doc/zmq.md: in 146ef599be outdated

18+set of specific notifiers. Currently there are notifiers that publish
19+blocks and transactions. This read-only facility requires only the
20+connection of a corresponding ZeroMQ subscriber port in receiving 
21+software; it is not authenticated nor is there any two-way protocol
22+involvement. Therefore, subscribers should validate the received data
23+since it may be out of date, incomplete or even invalid.

sipa commented at 10:05 pm on September 10, 2015:

The subscriber has no way to verify that the published block is invalid - it would need a full node for that, and that’s exactly what the ZMQ interface is for.

promag commented at 10:16 pm on September 10, 2015:

Yeah, reviewing and updating the doc.

sipa commented at 10:17 pm on September 10, 2015:

Thanks!

jtimon commented at 10:09 pm on September 10, 2015: contributor

Then only the documentation needs to be fixed? On Sep 10, 2015 6:02 PM, “Pieter Wuille” notifications@github.com wrote:

@promag https://github.com/promag If implemented correctly, there is no difference. You can present it as an optimization: “This informs you about the new chain tip, and as courtesy, we already give you the last block immediately so you don’t need to fetch it”. But I think the current interface gives the impression that it will relay all validated blocks, which is wrong and leads to incorrect understanding. The documentation even says so literally.

— Reply to this email directly or view it on GitHub #6103 (comment).

sipa commented at 10:11 pm on September 10, 2015: member

I think many people don’t grasp the difference between relaying blocks and chain tip updates, and the fact that this pull request was written with documentation that confuses the two is an indication of that. Don’t make the simplest way of using it the wrong one.

sipa commented at 10:14 pm on September 10, 2015: member

Anyway, it seems my opinion is controversial here. I’m fine with merging if the documentation is fixed, but I would really prefer to not have a mechanism that makes it this easy to misunderstand.

sipa commented at 10:18 pm on September 10, 2015: member

Also, sorry for hammering on this one detail. I’m very happy with the pull request overall and would like to see it merged soon.

promag commented at 10:19 pm on September 10, 2015: member

My point is that even -blocknotify leads to that problem, and it should be noted elsewhere that a reorg may not call the script for some blocks.

jtimon commented at 10:20 pm on September 10, 2015: contributor

Anyway, it seems my opinion is controversial here. I’m fine with merging if the documentation is fixed, but I would really prefer a mechanism that doesn’t make it this easy to misunderstand.

Assuming users read the updated docs, I don’t see how offering noth only-hash or full blocks is more confusing than only offering hash-only.

sipa commented at 10:21 pm on September 10, 2015: member

@jtimon Assuming users read the updated docs… @promag Agree, it could be clearer there too.

promag force-pushed on Sep 10, 2015

promag commented at 0:39 am on September 11, 2015: member

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

in src/zmq/zmqpublishnotifier.cpp: in 65b5df5c88 outdated

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

jonasschnelli commented at 7:03 am on September 11, 2015:

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

promag force-pushed on Sep 11, 2015

jonasschnelli commented at 7:15 am on September 11, 2015: contributor

Just git a build error on debian:

 0./configure --enable-debug --with-gui=no
 1[...]
 2checking whether to build ZMQ support... yes
 3checking for ZMQ... yes
 4[...]
 5make -j5
 6[...]
 7  CXX      wallet/libbitcoin_wallet_a-walletdb.o
 8zmq/zmqpublishnotifier.cpp: In function ‘int zmq_send_multipart(void*, const void*, size_t, ...)’:
 9zmq/zmqpublishnotifier.cpp:36:61: error: ‘zmq_msg_send’ was not declared in this scope
10         rc = zmq_msg_send(&msg, sock, data ? ZMQ_SNDMORE : 0);
11[...]
12zmq/zmqnotificationinterface.cpp:118:33: error: ‘zmq_ctx_destroy’ was not declared in this scope
13         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 commented at 7:20 am on September 11, 2015: contributor

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.

in src/zmq/zmqnotificationinterface.cpp: in 0197327276 outdated

 7+
 8+#include "version.h"
 9+#include "main.h"
10+#include "streams.h"
11+#include "util.h"
12+#include "netbase.h"

jonasschnelli commented at 7:22 am on September 11, 2015:

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

in src/zmq/zmqpublishnotifier.cpp: in 0197327276 outdated

0@@ -0,0 +1,175 @@
1+// Copyright (c) 2015 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include "zmqpublishnotifier.h"
6+#include "main.h"
7+#include "net.h"

jonasschnelli commented at 7:22 am on September 11, 2015:

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

jtimon commented at 4:29 pm on September 11, 2015:

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

sipa commented at 7:16 pm on September 11, 2015:

Whenever you use something defined by a header H in X.c, H needs to be #included by either X.c or X.h.

You should not remove #includes because they’re already included by one of the dependencies. That leads to unclear dependency graphs, and may cause breakage when dependencies change.

promag commented at 9:08 am on September 11, 2015: member

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 force-pushed on Sep 11, 2015

jgarzik commented at 4:09 pm on September 15, 2015: contributor

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 commented at 4:16 pm on September 15, 2015: contributor

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.

Add UpdatedBlockTip signal to CMainSignals and CValidationInterface 5624e055b3

Depends: Add ZeroMQ package 1136879df8

Add ZeroMQ support. Notify blocks and transactions via ZeroMQ

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

e6a14b64d6

QA: Add ZeroMQ RPC test 029e278286

promag force-pushed on Sep 16, 2015

promag commented at 10:33 am on September 16, 2015: member

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

jgarzik merged this on Sep 16, 2015

jgarzik closed this on Sep 16, 2015

jgarzik referenced this in commit 13b828270a on Sep 16, 2015

promag deleted the branch on Sep 16, 2015

luke-jr referenced this in commit 9190892342 on Oct 15, 2015

zkbot referenced this in commit 36df5a92f8 on Feb 9, 2017

zkbot referenced this in commit dd8b38316f on Feb 9, 2017

zkbot referenced this in commit 253c610783 on Feb 9, 2017

DrahtBot locked this on Sep 8, 2021

Add ZeroMQ notifications #6103