Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add ZeroMQ notifications #2050

Merged
merged 35 commits into from
Feb 9, 2017
Merged

Add ZeroMQ notifications #2050

merged 35 commits into from
Feb 9, 2017

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Jan 31, 2017

Cherry-picked from the following upstream PRs:

Closes #2020.

@str4d str4d added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-rpc-interface Area: RPC interface C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. labels Jan 31, 2017
@str4d str4d added this to the 1.0.6 milestone Jan 31, 2017
@str4d
Copy link
Contributor Author

str4d commented Jan 31, 2017

Before merging this, Buildbot needs to be updated to pip install zmq before running the RPC tests, as that is now a dependency.

@daira
Copy link
Contributor

daira commented Feb 3, 2017

contrib/debian/copyright needs an entry for depends/sources/zeromq-*.tar.gz.

@daira
Copy link
Contributor

daira commented Feb 4, 2017

Files: depends/sources/zeromq-*.tar.gz
Copyright:
 1994, 1995, 1996, 1999, 2000, 2001, 2002, 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
 2007-2014 iMatix Corporation
 2009-2011 250bpm s.r.o.
 2010-2011 Miru Limited
 2011 VMware, Inc.
 2012 Spotify AB
 2013 Ericsson AB
 2014 AppDynamics Inc.
 2015-2016 Brocade Communications Systems Inc.
License: LGPL-with-ZeroMQ-exception
License: LGPL-with-ZeroMQ-exception
 GNU LESSER GENERAL PUBLIC LICENSE
 Version 3, 29 June 2007
 .
 On Debian systems the GNU Lesser General Public License (LGPL) is
 located in '/usr/share/common-licenses/LGPL'.
 .
 This program is distributed in the hope that it will be useful,
 but WITHOUT ANY WARRANTY; without even the implied warranty of
 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 GNU General Public License for more details.
 --------------------------------------------------------------------------------
 SPECIAL EXCEPTION GRANTED BY COPYRIGHT HOLDERS
 .
 As a special exception, copyright holders give you permission to link this
 library with independent modules to produce an executable, regardless of
 the license terms of these independent modules, and to copy and distribute
 the resulting executable under terms of your choice, provided that you also
 meet, for each linked independent module, the terms and conditions of
 the license of that module. An independent module is a module which is not
 derived from or based on this library. If you modify this library, you must
 extend this exception to your version of the library.
  
 Note: this exception relieves you of any obligations under sections 4 and 5
 of this license, and section 6 of the GNU General Public License.
Comment:
 You should have received a copy of the GNU General Public License
 along with this program.  If not, see <http://www.gnu.org/licenses/>.

@daira
Copy link
Contributor

daira commented Feb 4, 2017

... and add libzmq3-dev libzmq5-dev (see here) to Build-Depends in contrib/debian/control. [Edit: fixed link.]

Edit: I'm not sure this is necessary. If it is not, then remove the reference to libzmq*-dev from doc/zmq.md.

@daira
Copy link
Contributor

daira commented Feb 4, 2017

The package.mk for libzmq builds it with the --without-libsodium --disable-curve options, which disables security and authentication features (I'm not sure of the exact details of what is disabled). Presumably this was intended by Bitcoin upstream because it's not the default, but is it what we want?

@daira daira added I-SECURITY Problems and improvements related to security. licensing labels Feb 4, 2017
@daira
Copy link
Contributor

daira commented Feb 4, 2017

libzmq has a new version 4.1.6. Its release notes since 4.1.5 are:

0MQ version 4.1.6 stable, released on 2016/11/01

Some of these look like significant correctness and Denial of Service issues.

@daira
Copy link
Contributor

daira commented Feb 4, 2017

zeromq/libzmq#2158 looks like undefined behaviour.

@daira
Copy link
Contributor

daira commented Feb 4, 2017

Oh, the current libzmq version is actually 4.2.1 (release notes; see also the release notes for 4.2.0). In any case we should upgrade. I'm not sure whether there were any incompatible API changes between 4.1.x and 4.2.x. Edit: the release notes claim compatibility.

@str4d
Copy link
Contributor Author

str4d commented Feb 6, 2017

@daira wrote:

... and add libzmq3-dev libzmq5-dev (see here) to Build-Depends in contrib/debian/control.

The link doesn't make sense to me - could you elaborate?

Edit: I'm not sure this is necessary. If it is not, then remove the reference to libzmq*-dev from doc/zmq.md.

The impression I got from that section was that it was listed so if a downstream packager wants to use provided dependencies instead of the depends/ system, they know which package to use. So I'm +0 on removing the reference.

@str4d
Copy link
Contributor Author

str4d commented Feb 6, 2017

@daira wrote:

The package.mk for libzmq builds it with the --without-libsodium --disable-curve options, which disables security and authentication features (I'm not sure of the exact details of what is disabled). Presumably this was intended by Bitcoin upstream because it's not the default, but is it what we want?

--without-libsodium was added while upgrading from 4.0.7 to 4.1.4 (bitcoin/bitcoin#7993) to fix a build error.

--disable-curve was added while upgrading to 4.1.5 (bitcoin/bitcoin#8238). Upstream did not need it enabled because they don't have libsodium as a dependency, and thus before 4.2 could not use CURVE security. The default security is null (no security), and upstream didn't enable plain (password-based).

From 4.2.0 --without-libsodium is the default, as tweetnacl is used instead.

Now, we could enable CURVE security, but I expect it's not really necessary because the ZMQ interface is generally used from an internal network.

@daira
Copy link
Contributor

daira commented Feb 6, 2017

ACK 4a8159343b0e1db2ff16dd138ab84271d2146774 and b418aa9027d3003a55089bdbcf2e6f55b46950f0.

@daira
Copy link
Contributor

daira commented Feb 6, 2017

@str4d wrote:

@daira wrote:

... and add libzmq3-dev libzmq5-dev (see here) to Build-Depends in contrib/debian/control.

The link doesn't make sense to me - could you elaborate?

Sorry, cut-and-paste error. The intended link was http://zeromq.org/distro:debian .

I think we should update doc/zmq.md to say libzmq5-dev and to make it clearer that this is only needed when not using the depends system.

@daira
Copy link
Contributor

daira commented Feb 6, 2017

@str4d wrote:

Now, we could enable CURVE security, but I expect it's not really necessary because the ZMQ interface is generally used from an internal network.

Hmm. Maybe we should just document (both in doc/zmq.md and doc/security-warnings.md) that ZMQ provides no authentication by default.

@bitcartel
Copy link
Contributor

Upstream still on 4.1.5. Maybe they'll upgrade to 4.2.1. bitcoin/bitcoin#8639

@bitcartel bitcartel added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2017
@zkbot
Copy link
Contributor

zkbot commented Feb 8, 2017

☔ The latest upstream changes (presumably #2072) made this pull request unmergeable. Please resolve the merge conflicts.

João Barbosa and others added 7 commits February 8, 2017 22:10
@str4d
Copy link
Contributor Author

str4d commented Feb 8, 2017

Rebased on master to fix merge conflict, and addressed @daira's comments.

Hmm. Maybe we should just document (both in doc/zmq.md and doc/security-warnings.md) that ZMQ provides no authentication by default.

This is already documented in doc/zmq.md by upstream (see here and here).

@bitcartel
Copy link
Contributor

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Feb 9, 2017

⌛ Trying commit edcec14 with merge 36df5a9...

@zkbot
Copy link
Contributor

zkbot commented Feb 9, 2017

💔 Test failed - zcash

@bitcartel
Copy link
Contributor

bitcartel commented Feb 9, 2017

@str4d See above:
=== Running testscript --mineblock ===
./qa/pull-tester/rpc-tests.sh: line 77: /home/admin/bbs/zcash/build/qa/rpc-tests/--mineblock: No such file or directory
!!! FAIL: --mineblock !!!

`${testScripts[@]}` now does split up `testscript --agument` in two elements pushed to the array (`testscript` and `--agument`).
@str4d
Copy link
Contributor Author

str4d commented Feb 9, 2017

@bitcartel there was an upstream PR I missed that fixed it.

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Feb 9, 2017

⌛ Trying commit 9bbc220 with merge dd8b383...

@zkbot
Copy link
Contributor

zkbot commented Feb 9, 2017

☀️ Test successful - zcash

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK

@bitcartel
Copy link
Contributor

ACK

@bitcartel
Copy link
Contributor

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Feb 9, 2017

📌 Commit 9bbc220 has been approved by bitcartel

@zkbot
Copy link
Contributor

zkbot commented Feb 9, 2017

⌛ Testing commit 9bbc220 with merge 253c610...

@zkbot
Copy link
Contributor

zkbot commented Feb 9, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit 9bbc220 into zcash:master Feb 9, 2017
This was referenced Feb 9, 2017
@str4d str4d mentioned this pull request Feb 9, 2017
@str4d str4d deleted the 2020-zmq branch January 27, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-interface Area: RPC interface C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. I-SECURITY Problems and improvements related to security. licensing S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.