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

pull mruddy wants to merge 1 commits into bitcoin:master from mruddy:zmq-deprecation-update changing 1 files +6 −2

mruddy commented at 11:25 PM on July 4, 2018: contributor

According to the libzmq API docs for version 4.2.3 (the version that we currently depend on) and later: http://api.zeromq.org/master:zmq-init is deprecated by http://api.zeromq.org/master:zmq-ctx-new http://api.zeromq.org/master:zmq-ctx-destroy is deprecated by http://api.zeromq.org/master:zmq-ctx-term

The one I/O thread set on zmq_init is the default for zmq_ctx_new, so no further change is necessary. I don't believe anything is changing beyond function naming with zmq_ctx_new and zmq_ctx_term -- the API docs read basically the same for the before and after functions.

I also added a log message to output the exact version of ZMQ being used by the node.
in src/zmq/zmqnotificationinterface.cpp:87 in 7a0ac394a6 outdated
```
  83 |          zmqError("Unable to initialize context");
  84 |          return false;
  85 |      }
  86 |  
  87 | +    int major = 0, minor = 0, patch = 0;
  88 | +    zmq_version(&major, &minor, &patch);
```
promag commented at 11:37 PM on July 4, 2018:

Can be called before context creation?

mruddy commented at 2:58 AM on July 5, 2018:

Yes, in practice, it seems that it can be. The reason why I still put it after context creation is because of this quote in the API docs at http://api.zeromq.org/master:zmq: Before using any ØMQ library functions you must create a ØMQ context.

edit: I just looked at the code. Yeah, I might have been too pedantic. As long as the API doc is not a contract, it's safe to move the call earlier: https://github.com/zeromq/libzmq/blob/master/src/zmq.cpp#L109-L114 Doesn't matter much to me. If it helps get it accepted, I'll change it.

promag commented at 6:17 AM on July 5, 2018:

It can matter if zmq_ctx_new fails for some reason, then you don't get the version in the logs.

mruddy commented at 11:20 AM on July 5, 2018:

Makes sense. Updated, thanks!
promag commented at 11:38 PM on July 4, 2018: member

Concept ACK.
fanquake added the label RPC/REST/ZMQ on Jul 4, 2018
fanquake commented at 12:17 AM on July 5, 2018: member

@mruddy ZeroMQ 4.2.3 is currently used in the depends system, but that doesn't necessarily mean it's the minimum version required to build Core. We don't currently have a minimum required version stated in the docs, but I assume the use of these functions should introduce one.

Having a quick look at the zmq api docs, it looks like they are both available from 3.2.x onwards. I can't see mention of either in the 2.2 stable or 3.1 branches.
mruddy commented at 3:11 AM on July 5, 2018: contributor

@fanquake Good point. Yep, they've been around for a while (since 2012 - 2013).

I found a mention in https://github.com/zeromq/libzmq/blob/master/NEWS in section 3.2.0 (RC1), released on 2012/06/05.

I also found that the commits for zmq_ctx_term https://github.com/zeromq/libzmq/commit/edd43e1ca45b86b649cbcbdada801b04d2895001 and zmq_ctx_new https://github.com/zeromq/libzmq/commit/6e71a54b1efe1ddb1805c6cc49e3f91492622a81 have the v4.2.0 tag. So, I'd say that's at least a minimum based on those two functions with this PR applied. I think it's reasonable to require a dependency to be at least that new. There has to be some other bug that requires a newer version, right :) ?
zmq: update to avoid deprecated zeromq api functions 7d16ac4d9a
mruddy force-pushed on Jul 5, 2018
fanquake commented at 11:05 PM on July 5, 2018: member

In that case, please update dependencies.md with what would be the new minimum required version.

I'd also suggest combining these changes into #13578, so that the zmq bump, testing, and any minimum version requirement discussion can happen together.
mruddy commented at 3:20 AM on July 6, 2018: contributor

@fanquake Sure, I'll combine this with #13578. I did more digging. We already require 4.x or later in the configure script. That aligns with Debian and Ubuntu packages for still supported releases (see https://packages.ubuntu.com/search?keywords=libzmq3-dev and https://packages.debian.org/search?keywords=libzmq3-dev). The nice thing is that the ZMQ project has repos for common distros that would make it easier for us to raise the minimum version (see http://zeromq.org/area:download and https://build.opensuse.org/repositories/network:messaging:zeromq:release-stable). Although, from what I've found, the functions that I'm switching to were around before, or with 4.0.0. We're already covered there. It's just a matter of whether to bump the minimum requirement, and if so, how far? I'm inclined to not mandate a minimum version bump right now. I do encourage using the latest version.
mruddy closed this on Jul 6, 2018
mruddy deleted the branch on Jul 6, 2018
mruddy cross-referenced this on Jul 6, 2018 from issue [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions by mruddy
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

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