zmq: Add support to listen on IPv6 addresses #22079

n-thumann commented at 9:30 PM on May 26, 2021: contributor

This PR adds support for listening on IPv6 addresses with bitcoinds ZMQ interface, just like the RPC server. Currently, it is not possible to specify an IPv6 address, as the ZMQ_IPV6 socket option is not set and therefore the ZMQ initialization fails, if one does so. The absence of this option has also been noted here. With this PR one can e.g. set -zmqpubhashblock=tcp://[::1]:28333 to listen on the IPv6 loopback address.

DrahtBot added the label RPC/REST/ZMQ on May 26, 2021

promag commented at 11:28 PM on May 26, 2021: member

Concept ACK.

laanwj commented at 11:37 AM on May 27, 2021: member

Concept ACK, it's kind of fascinating this went without IPv6 support for so long.

in doc/zmq.md:87 in 9e493c0aa5 outdated

  83 | @@ -84,6 +84,7 @@ For instance:
  84 |  
  85 |      $ bitcoind -zmqpubhashtx=tcp://127.0.0.1:28332 \
  86 |                 -zmqpubhashtx=tcp://192.168.1.2:28332 \
  87 | +               -zmqpubhashblock=tcp://[::1]:28333 \

promag commented at 9:09 AM on May 29, 2021:

nit , quote arg ='...'?

n-thumann commented at 6:53 PM on May 29, 2021:

Done! (with double instead of single quotes)

n-thumann force-pushed on May 29, 2021

theStack commented at 10:32 PM on May 30, 2021: contributor

Concept ACK

theStack commented at 9:31 PM on June 11, 2021: contributor

@n-thumann: Would you mind rebasing this PR on master? Then #22103 is included, which fixed the IPv6 detection for *BSDs and macOS systems.

n-thumann force-pushed on Jun 11, 2021

n-thumann commented at 10:01 PM on June 11, 2021: contributor

@n-thumann: Would you mind rebasing this PR on master? Then #22103 is included, which fixed the IPv6 detection for *BSDs and macOS systems.

Done ✌🏻

theStack commented at 11:41 PM on June 11, 2021: contributor

@n-thumann: Would you mind rebasing this PR on master? Then #22103 is included, which fixed the IPv6 detection for *BSDs and macOS systems.

Done ✌🏻

Thanks!

On my system (OpenBSD 6.9), the functional test interface_zmq.py doesn't seem to get any notifications in the subscriber sockets in the PR branch. Only if I remove settings the sockopt ZMQ_IPV6 from both server (bitcoind, publisher) and client (zmq interface Test, subscriber) it works again :/ would be interesting to know if this behaviour also shows up on other systems (that are not covered by the CI). Maybe it is connected to this ZMQ change: https://github.com/zeromq/libzmq/pull/2631 but that's just a vague guess...

n-thumann commented at 11:06 AM on June 13, 2021: contributor

On my system (OpenBSD 6.9), the functional test interface_zmq.py doesn't seem to get any notifications in the subscriber sockets in the PR branch. Only if I remove settings the sockopt ZMQ_IPV6 from both server (bitcoind, publisher) and client (zmq interface Test, subscriber) it works again :/ would be interesting to know if this behaviour also shows up on other systems (that are not covered by the CI). Maybe it is connected to this ZMQ change: zeromq/libzmq#2631 but that's just a vague guess...

I was able to confirm this behavior on OpenBSD, but not on any other OS (Debian, Ubuntu, macOS, FreeBSD). It looks like that OpenBSD is a bit picky, when it comes to setting the ZMQ_IPV6 option: While other OSes simply don't care (or bind dual-stacked), if the supplied address to listen on even is an IPv6 address, OpenBSD checks that. Therefore we must only set this option on OpenBSD, if it really is an IPv6 address. Same with the client side, so the functional test. If you want to have dual stack zmq on OpenBSD, you need to bind the socket twice, once for v4 and once for v6. @theStack feel free to test my latest changes 😊

theStack commented at 4:31 PM on June 13, 2021: contributor

It looks like that OpenBSD is a bit picky, when it comes to setting the ZMQ_IPV6 option: While other OSes simply don't care (or bind dual-stacked), if the supplied address to listen on even is an IPv6 address, OpenBSD checks that. Therefore we must only set this option on OpenBSD, if it really is an IPv6 address. Same with the client side, so the functional test. If you want to have dual stack zmq on OpenBSD, you need to bind the socket twice, once for v4 and once for v6. @theStack feel free to test my latest changes 😊

Very interesting, thanks for investigating deeper! I can confirm that the test succeeds with your latest changes. Looking at your fix, I'm wondering if we could simply always only set ZMQ_IPV6 if needed? That would avoid pre-processor conditionals depending on the OS (which is always kind of ugly and error-prone, considering that we check in code that is never compiled/run in the CI, in case of OpenBSD) and not cause troubles on other OSes that we didn't think of (e.g. DragonflyBSD?) but possibly have the same behaviour.

n-thumann commented at 10:24 PM on June 14, 2021: contributor

Looking at your fix, I'm wondering if we could simply always only set ZMQ_IPV6 if needed? That would avoid pre-processor conditionals depending on the OS (which is always kind of ugly and error-prone, considering that we check in code that is never compiled/run in the CI, in case of OpenBSD) and not cause troubles on other OSes that we didn't think of (e.g. DragonflyBSD?) but possibly have the same behavior.

Good point! I've ran some more tests and indeed DragonFly BSD is also affected. I also confirmed that only setting ZMQ_IPV6 if need has no negative impact / different behavior than always setting it on other OSes. Thus, I'll remove the pre-processor stuff 😊

<details> <summary>Raw test results</summary>

OS	always set option	only set option, if v6	listen address	socket type (netstat)	reachable via
macOS		X	127.0.0.1	tcp4	v4
macOS	X		127.0.0.1	tcp4	v4
macOS		X	::1	tcp6	v6
macOS	X		::1	tcp6	v6
macOS		X	0.0.0.0	tcp4	v4
macOS	X		0.0.0.0	tcp4	v4
macOS		X	::	tcp46	v4 & v6
macOS	X		::	tcp46	v4 & v6
---	---	---	---	---	---
Debian		X	127.0.0.1	tcp	v4
Debian	X		127.0.0.1	tcp6	v4
Debian		X	::1	tcp6	v6
Debian	X		::1	tcp6	v6
Debian		X	0.0.0.0	tcp	v4
Debian	X		0.0.0.0	tcp6	v4
Debian		X	::	tcp6	v4 & v6
Debian	X		::	tcp6	v4 & v6
---	---	---	---	---	---
FreeBSD		X	127.0.0.1	tcp4	v4
FreeBSD	X		127.0.0.1	tcp4	v4
FreeBSD		X	::1	tcp6	v6
FreeBSD	X		::1	tcp6	v6
FreeBSD		X	0.0.0.0	tcp	v4
FreeBSD	X		0.0.0.0	tcp4	v4
FreeBSD		X	::	tcp46	v4 & v6
FreeBSD	X		::	tcp46	v4 & v6
---	---	---	---	---	---
OpenBSD		X	127.0.0.1	tcp	v4
OpenBSD	X		127.0.0.1	fails	fails
OpenBSD		X	::1	tcp6	v6
OpenBSD	X		::1	tcp6	v6
OpenBSD		X	0.0.0.0	tcp	v4
OpenBSD	X		0.0.0.0	fails	fails
OpenBSD		X	::	tcp6	v6
OpenBSD	X		::	tcp6	v6
---	---	---	---	---	---
DragonFly BSD		X	127.0.0.1	tcp4	v4
DragonFly BSD	X		127.0.0.1	fails	fails
DragonFly BSD		X	::1	tcp6	v6
DragonFly BSD	X		::1	tcp6	v6
DragonFly BSD		X	0.0.0.0	tcp4	v4
DragonFly BSD	X		0.0.0.0	fails	fails
DragonFly BSD		X	::	tcp6	v6
DragonFly BSD	X		::	tcp6	v6

</details>

in src/zmq/zmqpublishnotifier.cpp:121 in e1820a5a6f outdated

 120 | +            const std::string ip = address.substr(tcpPrefix.length(), portColonIndex - tcpPrefix.length());
 121 | +            CNetAddr addr;
 122 | +            LookupHost(ip, addr, false);
 123 | +            if (!addr.IsIPv6())
 124 | +                enable_ipv6 = {0};
 125 | +        }

luke-jr commented at 7:41 PM on June 15, 2021:

This is pretty ugly. Can we just ignore errors setting ZMQ_IPV6 if the subsequent bind succeeds?

n-thumann commented at 8:32 PM on June 15, 2021:

I cleaned up a little bit, but I'm not sure, if ignoring an error while setting ZMQ_IPV6 is a good idea: If it fails, the binding of an IPv6 address will also fail and and prior error could help narrowing down the issue. And if we set the option on e.g. OpenBSD without checking, the bind will always fail for IPv4. Did I got you right? You want to always set ZMQ_IPV6 and see if the binding succeeds. If it does, all fine. If it didn't, what then?

luke-jr changes_requested

n-thumann force-pushed on Jun 15, 2021

theStack commented at 12:06 AM on June 16, 2021: contributor

7bbf6d8c92b still works here (OpenBSD 6.9) and the code looks cleaner than the previous version, with the pre-processor dependency gone. I tend to agree with @luke-jr that having to analyze the address string first in order to set (or not set) a socket option is "ugly", but on the other hand I don't see another solution.

Some suggestions:

Would add a short comment on why we only set ZMQ_IPV6 on IPv6 endpoints (like you had already in one of your earlier commits; you could simply take that and replace "OpenBSD" with "On some systems (like e.g. OpenBSD)...")
Could introduce a helper function like IsZMQAddressIPV6 that contains the IPv6 determination logic from the ZMQ address string; then you could simply write const int enable_ipv6 { IsZMQAddressIPV6(address) ? 1 : 0 };

n-thumann commented at 9:20 PM on June 16, 2021: contributor

I tend to agree with @luke-jr that having to analyze the address string first in order to set (or not set) a socket option is "ugly", but on the other hand I don't see another solution.

Absolutely! I'd be nice to have the plain address to bind, but the string entered by the user is the only thing to work with :/

Some suggestions: [...]

Applied!

theStack commented at 1:50 PM on June 18, 2021: contributor

@n-thumann: looks good to me. Could you squash at least the last two commits (or probably better even the last three, in order to avoid an "in-between" state where the "only set zmq_ipv6 if needed" fix has only been applied on one side), in order to ease review?

n-thumann force-pushed on Jun 18, 2021

n-thumann commented at 9:41 PM on June 18, 2021: contributor

Squashed :)

theStack changes_requested

theStack commented at 3:12 PM on June 19, 2021: contributor

Some feedback regarding coding style and structure (that I unfortunately missed before), regarding the first commit:

variables should be named in snake_case rather than (lower) camelCase (e.g. s/tcpIndex/tcp_index/)
"If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line."
Since the IsZMQAddressIPV6 helper doesn't use any instance specific data, I'd suggest to move it out of the class and simply put it in a static function on top of the file

zmq: Enable IPv6 on listening socket ded449b726

test: Add IPv6 test to zmq 8abe5703a9

doc: Add IPv6 address to zmq example e6998838e5

n-thumann force-pushed on Jun 20, 2021

n-thumann commented at 2:59 PM on June 20, 2021: contributor

Done! Btw, I really do appreciate your feedback!

theStack approved

theStack commented at 9:31 PM on June 20, 2021: contributor

Tested ACK e6998838e5548991274ad2bf1697d862905b8837 🌱 (reviewed code, built PR branch and ran included test on both OpenBSD 6.9 and Debian bullseye/sid, works as expected)

Done! Btw, I really do appreciate your feedback!

You're welcome. Looking forward to see more contributions from you in the future :-)

laanwj commented at 1:33 PM on September 9, 2021: member

Code review ACK e6998838e5548991274ad2bf1697d862905b8837

laanwj merged this on Sep 9, 2021

laanwj closed this on Sep 9, 2021

laanwj added the label Needs release note on Sep 9, 2021

sidhujag referenced this in commit 7ea979d93a on Sep 11, 2021

fanquake removed the label Needs release note on Sep 15, 2022

fanquake commented at 3:20 PM on September 15, 2022: member

This went into 23.0, but I'm not sure it actually got a release note. Removing the label.

jamesdorfman referenced this in commit 4181c95ce0 on Apr 21, 2023

bitcoin locked this on Sep 15, 2023