[depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions #13578

mruddy · 2018-06-30T14:54:01Z

Upgrade the ZeroMQ dependency from version 4.2.3 to the latest stable version 4.2.5.

This PR Follows the lead of #11986.

I upgraded both patch files to correspond to the version 4.2.5 libzmq files.
I assume doing so is still necessary and correct.

Without updating the patch line numbers, things appear to work, but you get extra log messages while building depends because things don't exactly match, e.g.:

/bitcoin/depends> make zeromq
Extracting zeromq...
/bitcoin/depends/sources/zeromq-4.2.5.tar.gz: OK
Preprocessing zeromq...
patching file src/windows.hpp
Hunk #1 succeeded at 58 (offset 3 lines).
patching file src/thread.cpp
Hunk #1 succeeded at 307 with fuzz 2 (offset 87 lines).
Hunk #2 succeeded at 323 with fuzz 2 (offset 90 lines).

Updating the patches seemed cleaner, so I did it. Note that libzmq had some whitespace changes, so that's why the updated patches do too.

More info: https://github.com/zeromq/libzmq/releases/tag/v4.2.5

tags: libzmq, zmq, 0mq

fanquake · 2018-07-01T04:37:40Z

@mruddy What's the motivation for bumping; is there a specific change/bugfix included in 4.2.3->4.2.5 that you require?

I've had a look at the patches, updating line numbers and formatting from upstream is fine.

Looks like we still also require the postprocess sed, as Libs.private: -lstdc++ is still included in 4.2.5's libzmq.pc.

Which platforms have you tested depends builds/gitian building on? Last time we updated zeromq we were stung with a sneaky change that broke gitian building, hence the need of the sed above.

fanquake · 2018-07-01T04:38:20Z

depends/patches/zeromq/0001-fix-build-with-older-mingw64.patch

@@ -1,17 +1,17 @@
-From 1a159c128c69a42d90819375c06a39994f3fbfc1 Mon Sep 17 00:00:00 2001


Given that these are just being updated for upstream, you could leave the original From/Date

the date didn't change. the thing that changed was the hex value, which is only the local commit made to my libzmq repo so that i could run git format-patch master --stdout to get a patch that had most of the same formatting as the original one. it's a pretty meaningless line. i was actually thinking of just using the git diff output for the patch file contents as that would make these diffs easier in the future, but then you lose the commit comment being in the patch file.

mruddy · 2018-07-01T12:36:44Z

@fanquake My primary motivation is that my dev machine is Ubuntu 18.04 LTS and its libzmq5 package is already v4.2.5. Without building against depends, I was already using 4.2.5. Since I did the looking to figure that out, I figured that I'd make this update. This is the only machine where I tested the depends build as well.

From the 4.2.4 release notes, this one looked a little interesting. Although, I did NOT try to use it to make my node crash, so I don't know if it actually applies: "ZMQ_PUB crash when due to high volume of subscribe and unsubscribe messages, an unmatched unsubscribe message is received in certain conditions".

mruddy · 2018-07-06T03:28:58Z

Combined #13596 into this PR.
In summary:

Upgrade ZeroMQ from 4.2.3 to 4.2.5 (latest stable).
Centralize ZeroMQ version documentation into dependencies.md.
Note the already existing minimum ZMQ version requirement of 4.0.0 (enforced by configure.ac) which already supports switching to use the non-deprecated API functions.
Log what version of the ZMQ lib (libzmq) is used by the node in the zmq debug log messages.

mruddy · 2018-07-16T20:51:04Z

Just noting that I think this PR is ready to merge. The current latest commit 473a48cae5d63f5c0baee4990756d4a9b18c4b5f covers all feedback received.

jmcorgan · 2018-07-16T21:50:04Z

utACK

greenaddress · 2018-07-17T15:32:42Z

I know is late for this but it would be great if we could also add this commit as a patch zeromq/libzmq@58d1339

If you want to squash it in your PR i rebased it against yours in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched

I also prepared a separate PR in #13689

laanwj · 2018-07-18T14:17:46Z

utACK 473a48cae5d63f5c0baee4990756d4a9b18c4b5f

laanwj · 2018-07-18T14:19:38Z

I think in general, though, it makes sense to avoid small version bumps for libraries such as zeromq unless there's a strong motivation to do so (such as a bug that affects us). This PR is good, but let's not make it a habit.

mruddy · 2018-07-18T21:22:45Z

@laanwj OK and thanks for reviewing this.

@greenaddress Just wanted to let you know that I'm not ignoring you, I've just been following the conversation over at #13689. I think I'd rather just leave them separate since this PR already has some ACKs and it looks like you're pretty well done with your changes in the other PR too. Thanks for your review too.

a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum) Pull request description: This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq. For reference see #11844 (comment) ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation. This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](#13689 (comment)) ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only #11986 as non-necessary but otherwise unreleased.~~ In the likely case #13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one. Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00

DrahtBot · 2018-08-02T16:08:47Z

No more conflicts as of last run.

DrahtBot · 2018-08-06T16:10:05Z

Needs rebase

mruddy · 2018-08-06T21:14:55Z

rebased

mruddy · 2018-08-23T11:59:29Z

This is ready to merge.

ken2812221

Maybe you did something wrong when you rebase this. Otherwise utACK

ken2812221 · 2018-08-23T12:57:41Z

doc/build-unix.md

@@ -47,7 +47,7 @@ Optional dependencies:
 protobuf    | Payments in GUI  | Data interchange format used for payment protocol (only needed when GUI enabled)
 libqrencode | QR codes in GUI  | Optional for generating QR codes (only needed when GUI enabled)
 univalue    | Utility          | JSON parsing and encoding (bundled version will be used unless --with-system-univalue passed to configure)
- libzmq3     | ZMQ notification | Optional, allows generating ZMQ notifications (requires ZMQ version >= 4.x)
+ libzmq3     | ZMQ notification | Optional, allows generating ZMQ notifications


Why change this?

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

I'm not convinced that removing this information everywhere is a good idea.
Yes, you are correct that the information is in dependencies.md, however I still think, for users actually reading this documentation, it is useful to mention the major version requirement in other places too.

ok, i've mostly re-duplicated the version documentation and rebased and squashed it all again.

ken2812221 · 2018-08-23T12:58:01Z

doc/build-unix.md

@@ -93,7 +93,7 @@ Optional (see --with-miniupnpc and --enable-upnp-default):

    sudo apt-get install libminiupnpc-dev

-ZMQ dependencies (provides ZMQ API 4.x):
+ZMQ dependencies (provides ZMQ API):


Why change this?

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

ken2812221 · 2018-08-23T13:00:52Z

doc/zmq.md

@@ -33,8 +33,10 @@ buffering or reassembly.

 ## Prerequisites

-The ZeroMQ feature in Bitcoin Core requires ZeroMQ API version 4.x or
-newer. Typically, it is packaged by distributions as something like
+The ZeroMQ feature in Bitcoin Core requires the ZeroMQ API


Why get rid of version limit?

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

ken2812221 · 2018-08-23T13:01:45Z

doc/zmq.md

@@ -78,7 +80,7 @@ bytes).
 These options can also be provided in bitcoin.conf.

 ZeroMQ endpoint specifiers for TCP (and others) are documented in the
-[ZeroMQ API](http://api.zeromq.org/4-0:_start).
+[ZeroMQ API](http://api.zeromq.org/).


Why change this?

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

…mq api functions

ken2812221 · 2018-09-17T00:04:08Z

utACK f1bd03e

…eprecated zeromq api functions f1bd03e [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions (mruddy) Pull request description: Upgrade the ZeroMQ dependency from version 4.2.3 to the latest stable version 4.2.5. This PR Follows the lead of #11986. I upgraded both patch files to correspond to the version `4.2.5` libzmq files. I assume doing so is still necessary and correct. Without updating the patch line numbers, things appear to work, but you get extra log messages while building `depends` because things don't exactly match, e.g.: ``` /bitcoin/depends> make zeromq Extracting zeromq... /bitcoin/depends/sources/zeromq-4.2.5.tar.gz: OK Preprocessing zeromq... patching file src/windows.hpp Hunk #1 succeeded at 58 (offset 3 lines). patching file src/thread.cpp Hunk #1 succeeded at 307 with fuzz 2 (offset 87 lines). Hunk #2 succeeded at 323 with fuzz 2 (offset 90 lines). ``` Updating the patches seemed cleaner, so I did it. Note that libzmq had some whitespace changes, so that's why the updated patches do too. More info: https://github.com/zeromq/libzmq/releases/tag/v4.2.5 tags: libzmq, zmq, 0mq Tree-SHA512: 78659dd276b5311e40634b1bbebb802ddd6b69662ba3c84995ef1e3795c49a78b1635112c7fd72a405ea36e2cc3bdeb84e6d00d4e491a349bba1dafff50e2fa5

Summary: ...api functions Backport of core [[bitcoin/bitcoin#13578 | PR13578]]. The minimum version is set in accordance to our dependencies doc to 4.1.5. Depends on D5599 and D5600 (for the test plan). Test Plan: ninja all check ./test/functional/test_runner.py interface_zmq Run the Gitian builds. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5603

a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum) Pull request description: This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq. For reference see bitcoin#11844 (comment) ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation. This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment)) ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~ In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one. Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00

Zcash: Backported from bitcoin/bitcoin#13578

Update ZeroMQ Includes changes cherry-picked from the following upstream PRs: - bitcoin/bitcoin#9254 - bitcoin/bitcoin#13578 - bitcoin/bitcoin#15844 - bitcoin/bitcoin#16370 - Only the ZeroMQ changes. - bitcoin/bitcoin#16949

a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum) Pull request description: This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq. For reference see bitcoin#11844 (comment) ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation. This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment)) ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~ In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one. Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00

Summary: ...api functions Backport of core [[bitcoin/bitcoin#13578 | PR13578]]. The minimum version is set in accordance to our dependencies doc to 4.1.5. Depends on D5599 and D5600 (for the test plan). Test Plan: ninja all check ./test/functional/test_runner.py interface_zmq Run the Gitian builds. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5603

…avoid deprecated zeromq api functions f1bd03e [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions (mruddy) Pull request description: Upgrade the ZeroMQ dependency from version 4.2.3 to the latest stable version 4.2.5. This PR Follows the lead of bitcoin#11986. I upgraded both patch files to correspond to the version `4.2.5` libzmq files. I assume doing so is still necessary and correct. Without updating the patch line numbers, things appear to work, but you get extra log messages while building `depends` because things don't exactly match, e.g.: ``` /bitcoin/depends> make zeromq Extracting zeromq... /bitcoin/depends/sources/zeromq-4.2.5.tar.gz: OK Preprocessing zeromq... patching file src/windows.hpp Hunk dashpay#1 succeeded at 58 (offset 3 lines). patching file src/thread.cpp Hunk dashpay#1 succeeded at 307 with fuzz 2 (offset 87 lines). Hunk dashpay#2 succeeded at 323 with fuzz 2 (offset 90 lines). ``` Updating the patches seemed cleaner, so I did it. Note that libzmq had some whitespace changes, so that's why the updated patches do too. More info: https://github.com/zeromq/libzmq/releases/tag/v4.2.5 tags: libzmq, zmq, 0mq Tree-SHA512: 78659dd276b5311e40634b1bbebb802ddd6b69662ba3c84995ef1e3795c49a78b1635112c7fd72a405ea36e2cc3bdeb84e6d00d4e491a349bba1dafff50e2fa5

fanquake added the Build system label Jul 1, 2018

fanquake reviewed Jul 1, 2018

View reviewed changes

fanquake mentioned this pull request Jul 5, 2018

zmq: update to avoid deprecated zeromq api functions and log zmq version used #13596

Closed

mruddy force-pushed the zmq-upgrade branch from db8096a to 473a48c Compare July 6, 2018 03:24

mruddy changed the title ~~[depends] zeromq 4.2.5~~ [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions Jul 6, 2018

DrahtBot mentioned this pull request Jul 6, 2018

depends: Add RISC-V support #13543

Merged

This was referenced Jul 17, 2018

[depends] zeromq 4.2.3 #11986

Merged

depends: disable Werror when building zmq #13689

Merged

DrahtBot mentioned this pull request Aug 2, 2018

doc: correct versions in dependencies.md #13853

Merged

DrahtBot added the Needs rebase label Aug 6, 2018

mruddy force-pushed the zmq-upgrade branch from 473a48c to 33d997d Compare August 6, 2018 21:14

fanquake removed the Needs rebase label Aug 6, 2018

maflcko added Needs gitian build and removed Needs gitian build labels Aug 16, 2018

DrahtBot mentioned this pull request Aug 21, 2018

ZMQ: Use C++ language bindings #14017

Closed

DrahtBot removed the Needs gitian build label Aug 22, 2018

ken2812221 suggested changes Aug 23, 2018

View reviewed changes

[depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zero…

f1bd03e

…mq api functions

mruddy force-pushed the zmq-upgrade branch from 33d997d to f1bd03e Compare September 11, 2018 20:35

laanwj merged commit f1bd03e into bitcoin:master Sep 17, 2018

mruddy deleted the zmq-upgrade branch September 17, 2018 13:00

bitcoin deleted a comment from DrahtBot Sep 17, 2018

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

str4d pushed a commit to str4d/zcash that referenced this pull request Oct 5, 2020

[depends, zmq, doc] avoid deprecated zeromq api functions

edfb4d9

Zcash: Backported from bitcoin/bitcoin#13578

str4d mentioned this pull request Oct 5, 2020

Update ZeroMQ zcash/zcash#4766

Merged

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

		@@ -1,17 +1,17 @@
		From 1a159c128c69a42d90819375c06a39994f3fbfc1 Mon Sep 17 00:00:00 2001

[depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions #13578

[depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions #13578

Uh oh!

Conversation

mruddy commented Jun 30, 2018

Uh oh!

fanquake commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mruddy commented Jul 1, 2018

Uh oh!

mruddy commented Jul 6, 2018

Uh oh!

mruddy commented Jul 16, 2018

Uh oh!

jmcorgan commented Jul 16, 2018

Uh oh!

greenaddress commented Jul 17, 2018

Uh oh!

laanwj commented Jul 18, 2018

Uh oh!

laanwj commented Jul 18, 2018

Uh oh!

mruddy commented Jul 18, 2018

Uh oh!

DrahtBot commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 6, 2018

Uh oh!

mruddy commented Aug 6, 2018

Uh oh!

mruddy commented Aug 23, 2018

Uh oh!

ken2812221 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Sep 17, 2018

Uh oh!

Uh oh!

DrahtBot commented Aug 2, 2018 •

edited

Loading