zmq: Add support to listen on multiple interfaces #18309

n-thumann · 2020-03-10T15:42:57Z

This PR adds support for ZeroMQ to listen on multiple interfaces, just like the RPC server.
Currently, if you specify more than one e.g. zmqpubhashblock paramter, only the first one will be used. Therefore a user may be forced to listen on all interfaces (e.g. zmqpubhashblock=0.0.0.0:28332), which can result in an increased attack surface.
With this PR a user can specify multiple interfaces to listen on, e.g.
-zmqpubhashblock=tcp://127.0.0.1:28332 -zmqpubhashblock=tcp://192.168.1.123:28332.

DrahtBot · 2020-03-10T20:40:06Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

promag

Concept ACK.

Maybe use a minimal diff

     for (const auto& entry : factories)
     {
         std::string arg("-zmq" + entry.first);
-        if (gArgs.IsArgSet(arg))
-        {
-            CZMQNotifierFactory factory = entry.second;
-            std::string address = gArgs.GetArg(arg, "");
+        CZMQNotifierFactory factory = entry.second;
+        for (const std::string& address : gArgs.GetArgs(arg)) {
             CZMQAbstractNotifier *notifier = factory();
             notifier->SetType(entry.first);
             notifier->SetAddress(address);

Also:

update the doc/zmq.md with this change
add release note too
update some test in test/functional/interface_zmq.py with multiple interfaces.

Welcome!

promag · 2020-03-11T08:05:06Z

update some test in test/functional/interface_zmq.py with multiple interfaces.

@n-thumann don't forget about this.

n-thumann · 2020-03-11T08:59:54Z

update some test in test/functional/interface_zmq.py with multiple interfaces.

@n-thumann don't forget about this.

Thank you for your feedback! The test is already in the works :)

n-thumann · 2020-03-11T18:16:13Z

Currently the debug.log prints
2020-03-11T17:52:01.715152Z [scheduler] zmq: Publish hashblock <blockhash> 2020-03-11T17:52:01.715248Z [scheduler] zmq: Publish hashblock <blockhash>, when hashblock is triggered and ZeroMQ is listening on two interfaces.
Do you think it´s useful to append something like to tcp://127.0.0.1:28332 to each log entry, so that a user knows why there is an apparently duplicate entry?

promag · 2020-03-11T18:31:35Z

Do you think it´s useful to append something like to tcp://127.0.0.1:28332 to each log entry, so that a user knows why there is an apparently duplicate entry?

Yes, makes sense in this change. Alternatively it could avoid printing the duplicate messages but not worth it.

test/functional/interface_zmq.py

mruddy · 2020-04-21T18:49:01Z

Tested ACK

I tested on Ubuntu 19.10 x86_64 with zmq version 4.3.2.

I verified that the node starts when not specifying any of the zmq flags, when starting with between 1 and 3 -zmqpubhashblock on different addresses and/or ports, with and without -zmqpubhashblockhwm=10000. I verified log entries, and I also verified receiving messages with the contrib/zmq/zmq_sub.py subscriber.

@n-thumann side note, you might have the right skill set to review my #14687 related to zmq, if you have some spare cycles. that's a small change that the lightning guys have been requesting for a while. thanks!

instagibbs · 2020-09-02T01:47:21Z

concept ACK, will review

instagibbs

utACK 6b8d300

Small, straight forward change. We can clean up log messages later if they're confusing.

test/functional/interface_zmq.py

instagibbs · 2020-09-02T12:56:48Z

re-utACK ab996c2

jonatack · 2020-09-04T12:08:10Z

Concept ACK

n-thumann · 2020-09-30T22:05:41Z

Adjusted log output to new notification types of #19572.

luke-jr

utACK

laanwj · 2020-10-01T15:42:18Z

Code review ACK e66870c

instagibbs · 2020-10-01T15:42:42Z

reACK e66870c

e66870c zmq: Append address to notify log output (nthumann) 241803d test: Add zmq test to support multiple interfaces (nthumann) a0b2e5c doc: Add release notes to support multiple interfaces (nthumann) b1c3f18 doc: Adjust ZMQ usage to support multiple interfaces (nthumann) 347c94f zmq: Add support to listen on multiple interfaces (Nicolas Thumann) Pull request description: This PR adds support for ZeroMQ to listen on multiple interfaces, just like the RPC server. Currently, if you specify more than one e.g. `zmqpubhashblock` paramter, only the first one will be used. Therefore a user may be forced to listen on all interfaces (e.g. `zmqpubhashblock=0.0.0.0:28332`), which can result in an increased attack surface. With this PR a user can specify multiple interfaces to listen on, e.g. `-zmqpubhashblock=tcp://127.0.0.1:28332 -zmqpubhashblock=tcp://192.168.1.123:28332`. ACKs for top commit: laanwj: Code review ACK e66870c instagibbs: reACK bitcoin@e66870c Tree-SHA512: f38ab4a6ff00dc821e5f4842508cefadb701e70bb3893992c1b32049be20247c8aa9476a1f886050c5f17fe7f2ce99ee30193ce2c81a7482a5a51f8fc22300c7

Summary: > This PR adds support for ZeroMQ to listen on multiple interfaces, just like the RPC server. > Currently, if you specify more than one e.g. zmqpubhashblock paramter, only the first one will be used. Therefore a user may be forced to listen on all interfaces (e.g. zmqpubhashblock=0.0.0.0:28332), which can result in an increased attack surface. > With this PR a user can specify multiple interfaces to listen on, e.g. > -zmqpubhashblock=tcp://127.0.0.1:28332 -zmqpubhashblock=tcp://192.168.1.123:28332. This is a backport of [[bitcoin/bitcoin#18309 | core#18309]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10339

DrahtBot added the RPC/REST/ZMQ label Mar 10, 2020

promag reviewed Mar 10, 2020

View reviewed changes

n-thumann force-pushed the zmq-listen-multiple-interfaces branch from 582d81e to 2dcb4df Compare March 10, 2020 22:51

DrahtBot mentioned this pull request Mar 11, 2020

ZMQ: Small cleanups in the ZMQ code #13686

Merged

promag reviewed Mar 11, 2020

View reviewed changes

test/functional/interface_zmq.py Outdated Show resolved Hide resolved

n-thumann force-pushed the zmq-listen-multiple-interfaces branch 2 times, most recently from d6f80e8 to 751a5c5 Compare March 11, 2020 18:52

n-thumann requested a review from promag March 16, 2020 16:26

laanwj added the Feature label Mar 19, 2020

mruddy mentioned this pull request Apr 21, 2020

zmq: shutdown/abort when configured with invalid addresses #15198

Closed

This was referenced Jul 7, 2020

Add zmq notifications for evicted transactions, tests #19462

Closed

Expand functional zmq transaction tests #19507

Merged

DrahtBot mentioned this pull request Jul 23, 2020

ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572

Merged

DrahtBot added the Needs rebase label Aug 31, 2020

n-thumann force-pushed the zmq-listen-multiple-interfaces branch from 751a5c5 to 6b8d300 Compare August 31, 2020 21:20

DrahtBot removed the Needs rebase label Aug 31, 2020

instagibbs approved these changes Sep 2, 2020

View reviewed changes

test/functional/interface_zmq.py Outdated Show resolved Hide resolved

n-thumann force-pushed the zmq-listen-multiple-interfaces branch from 6b8d300 to ab996c2 Compare September 2, 2020 08:14

n-thumann mentioned this pull request Sep 2, 2020

random wallet_basic failure #19853

Closed

DrahtBot added the Needs rebase label Sep 19, 2020

n-thumann force-pushed the zmq-listen-multiple-interfaces branch from ab996c2 to d8ecf73 Compare September 20, 2020 12:38

DrahtBot added Needs rebase and removed Needs rebase labels Sep 20, 2020

n-thumann added 3 commits September 23, 2020 23:14

zmq: Add support to listen on multiple interfaces

347c94f

doc: Adjust ZMQ usage to support multiple interfaces

b1c3f18

doc: Add release notes to support multiple interfaces

a0b2e5c

n-thumann force-pushed the zmq-listen-multiple-interfaces branch from d8ecf73 to f50cd3c Compare September 23, 2020 21:23

DrahtBot removed the Needs rebase label Sep 23, 2020

n-thumann force-pushed the zmq-listen-multiple-interfaces branch from f50cd3c to d39d889 Compare September 30, 2020 22:03

n-thumann added 2 commits October 1, 2020 00:33

test: Add zmq test to support multiple interfaces

241803d

zmq: Append address to notify log output

e66870c

n-thumann force-pushed the zmq-listen-multiple-interfaces branch from d39d889 to e66870c Compare September 30, 2020 22:34

luke-jr approved these changes Oct 1, 2020

View reviewed changes

laanwj merged commit a0185d9 into bitcoin:master Oct 1, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

zmq: Add support to listen on multiple interfaces #18309

zmq: Add support to listen on multiple interfaces #18309

Uh oh!

Conversation

n-thumann commented Mar 10, 2020

Uh oh!

DrahtBot commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 11, 2020

Uh oh!

n-thumann commented Mar 11, 2020

Uh oh!

n-thumann commented Mar 11, 2020

Uh oh!

promag commented Mar 11, 2020

Uh oh!

Uh oh!

mruddy commented Apr 21, 2020

Uh oh!

instagibbs commented Sep 2, 2020

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Sep 2, 2020

Uh oh!

jonatack commented Sep 4, 2020

Uh oh!

n-thumann commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 1, 2020

Uh oh!

instagibbs commented Oct 1, 2020

Uh oh!

Uh oh!

DrahtBot commented Mar 10, 2020 •

edited

Loading

n-thumann commented Sep 30, 2020 •

edited

Loading