zmq: enable tcp keepalive

mruddy commented at 12:05 pm on November 8, 2018: contributor

This addresses #12754.

These changes enable node operators to address the silent dropping (by network middle boxes) of long-lived low-activity ZMQ TCP connections via further operating system level TCP keepalive configuration. For example, ZMQ sockets that publish block hashes can be affected in this way due to the length of time it sometimes takes between finding blocks (e.g.- sometimes more than an hour).

Prior to this patch, operating system level TCP keepalive configurations would not take effect since the SO_KEEPALIVE option was not enabled on the underlying socket.

There are additional ZMQ socket options related to TCP keepalive that can be set. However, I decided not to implement those options in this changeset because doing so would require adding additional bitcoin node configuration options, and would not yield a better outcome. I preferred a small, easily reviewable patch that doesn’t add a bunch of new config options, with the tradeoff that the fine tuning would have to be done via well-documented operating system specific configurations.

I tested this patch by running a node with: ./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=tcp://127.0.0.1:28332 & and connecting to it with: python3 ./contrib/zmq/zmq_sub.py

Without these changes, ss -panto | grep 28332 | grep ESTAB | grep bitcoin will report no keepalive timer information. With these changes, the output from the prior command will show keepalive timer information consistent with the configuration at the time of connection establishment, e.g.-: timer:(keepalive,119min,0).

I also tested with a non-TCP transport and did not witness any adverse effects: ./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=ipc:///tmp/bitcoin.block &

fanquake added the label RPC/REST/ZMQ on Nov 8, 2018

MarcoFalke deleted a comment on Nov 8, 2018

in doc/zmq.md:112 in e7921ce307 outdated

107+to lower the keepalive setting to 10 minutes:
108+
109+sudo sysctl -w net.ipv4.tcp_keepalive_time=600
110+
111+Setting the keepalive values appropriately for your operating environment may
112+improve connectiviy in situations where long-lived connections are silently

practicalswift commented at 10:31 am on November 9, 2018:

“connectivity” :-)

mruddy commented at 11:13 am on November 9, 2018:

fixed, thanks :-)

mruddy force-pushed on Nov 9, 2018

laanwj commented at 12:57 pm on November 12, 2018: member

Looks good to me, but probably needs to be tested by @bitkevin whether it really solves #12754.

in src/zmq/zmqpublishnotifier.cpp:91 in 66609de444 outdated

85@@ -86,6 +86,15 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
86             return false;
87         }
88 
89+        const int so_keepalive_option {1};
90+        rc = zmq_setsockopt(psocket, ZMQ_TCP_KEEPALIVE, &so_keepalive_option, sizeof(so_keepalive_option));
91+        if (rc != 0)

promag commented at 11:30 am on November 15, 2018:

nit, { here.

mruddy commented at 1:18 pm on November 15, 2018:

updated the formatting here to be on the same line. i had to force myself to do it because that’s what the dev notes say to do. but it was hard because so much of the zmq code is of the other style :)

in doc/zmq.md:109 in 66609de444 outdated

104+operating system configuration and must be configured prior to connection establishment.
105+
106+For example, when running on GNU/Linux, one might use the following
107+to lower the keepalive setting to 10 minutes:
108+
109+sudo sysctl -w net.ipv4.tcp_keepalive_time=600

promag commented at 11:52 am on November 15, 2018:

Does it make sense to support setting per socket? I mean, allow custom ZMQ_TCP_KEEPALIVE_IDLE?

mruddy commented at 1:24 pm on November 15, 2018:

The best reason that I could think of to support my stance of not wanting to add the extra options, besides adding complexity and configuration bloat, is that these timeouts are environmental and so it seems like once you figure them out, it’s best to just set an appropriate value at the system level. This does assume that you’re not making these ZMQ connections to a bunch of other external-party system environments where per socket config could be useful. I think this assumption is valid though because I perceive the ZMQ stuff to be for data distribution within one’s own (internal) environment. Third parties would be running their own nodes and distributing data from those nodes within their own envs. Make sense?

promag commented at 12:04 pm on November 15, 2018: member

Like changing tcp_keepalive_time globally, is it possible to enable keepalive globally?

mruddy commented at 1:12 pm on November 15, 2018: contributor

is it possible to enable keepalive globally?

I don’t believe it is for Linux or Windows.

promag commented at 1:15 pm on November 15, 2018: member

@mruddy thanks, I also don’t find evidence that it’s possible.

zmq: enable tcp keepalive c276df7759

mruddy force-pushed on Nov 15, 2018

in src/zmq/zmqpublishnotifier.cpp:93 in c276df7759

85@@ -86,6 +86,14 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
86             return false;
87         }
88 
89+        const int so_keepalive_option {1};
90+        rc = zmq_setsockopt(psocket, ZMQ_TCP_KEEPALIVE, &so_keepalive_option, sizeof(so_keepalive_option));
91+        if (rc != 0) {
92+            zmqError("Failed to set SO_KEEPALIVE");
93+            zmq_close(psocket);

luke-jr commented at 5:19 am on December 20, 2018:

Maybe if it fails to set SO_KEEPALIVE, it should still continue with a warning?

laanwj commented at 4:12 pm on January 8, 2019:

When does this fail? If SO_KEEPALIVE is important, I don’t think ignoring errors is a good idea. Not everyone continuously monitors the logs, and it might suddenly result in a return of issue #14687.

jgarzik commented at 5:15 pm on January 8, 2019:

FWIW,

The kernel only fails SO_KEEPALIVE setsockopt(2) if there is something assert()-level wrong with the socket (invalid file descriptor; fd is closed; fd is not a socket; basic stuff…)
zmq_setsockopt() is not a direct wrapper, but close: the value is stored, and setsockopt(2) is called later in zmq’s src/tcp.cpp.
zmq_setsockopt() will only return an error if there’s something assert()-level wrong with the zmq structure.

Ergo, errors are serious but will not occur due to Linux network stack conditions. Errors only occur due to programmatic error (zmq passed file fd to socket syscall) or memory corruption.

luke-jr commented at 7:30 pm on January 8, 2019:

I would imagine it fails on (perhaps just hypothetical) systems without SO_KEEPALIVE

jgarzik commented at 9:18 pm on January 8, 2019:

zmq will return 0 (success) on systems without SO_KEEPALIVE

dlogicman commented at 11:02 am on March 13, 2019: none

Tested ACK.

I am running lightningnetwork/lnd in connection with bitcoind on Ubuntu 16.04 and bitcoind dropped the zmq connection always after a few hours (according to output of ss -panto). Therefore the status of synced_to_chain in lnd switched to false.

To resolve this problem I included this patch into zmqpublishnotifier.cpp on top of bitcoind 0.17.1 and lowered the tcp_keepalive_time (sudo sysctl -w net.ipv4.tcp_keepalive_time=60). Since restarting the patched bitcoind, the connection is stable.

Crosscheck: I switched to the unpatched 0.18.0rc1 yesterday and after a few minutes bitcoind dropped the zmq connection again. I returned to the patched 0.17.1 and the connection is since stable again.

mruddy commented at 5:34 pm on March 13, 2019: contributor

@dlogemann thanks for testing!

I am still happy with this patch as well. I think it’s ready to merge.

dannybabbev commented at 3:53 am on May 1, 2019: none

Hi, when will this PR be merged?

jonasschnelli commented at 8:28 am on May 2, 2019: contributor

Needs more review.

DrahtBot closed this on Aug 16, 2019

DrahtBot commented at 1:49 pm on August 16, 2019: member

DrahtBot reopened this on Aug 16, 2019

Haaroon commented at 2:14 pm on December 3, 2019: none

So this was never added? Cause there are still issues with zmq and lnd.

laanwj commented at 3:14 pm on December 3, 2019: member

So this was never added? Cause there are still issues with zmq and lnd.

so help review and test this PR, and post an ACK

Haaroon commented at 11:37 am on December 4, 2019: none

I’ve cherry picked this up into the latest bitcoind version, been running it for the past 15 hours. So far its working great. My lnd node has stayed synced to the network, channels have been opened and persisted fine. The actual zmq code base changes are minimal so seems ok to be pushed into master. However I’ve only tested this on ubuntu.

n-thumann commented at 11:36 am on April 23, 2020: contributor

Tested ACK: Works as expected (sends TCP Keepalive packets every net.ipv4.tcp_keepalive_time seconds). I´ve tested on Debian Buster by subscribing with contrib/zmq/zmq_sub.py from another server and not sending anything for seven hours behind the NATed connection 👍

mruddy commented at 12:09 pm on May 5, 2020: contributor

Just adding some notes from my testing with Windows 10 Home Version 1909 OS build 18363.418 that was downloaded from https://www.microsoft.com/en-us/software-download/windows10ISO

0sha256sum Win10_1909_English_x64.iso
101bf1eb643f7e50d0438f4f74fb91468d35cde2c82b07abc1390d47fc6a356be Win10_1909_English_x64.iso

I cross-compiled using the directions at https://github.com/bitcoin/bitcoin/blob/master/doc/build-windows.md.

In summary, these changes do not affect the Windows 10 regtest Bitcoin node (i.e.- where the Windows Bitcoin node is the ZMQ publisher). I think the reason why is because the libzmq code does not call setsockopt with SO_KEEPALIVE due to an old commit in the libzmq repo for Windows. I think that commit should have set the SIO_KEEPALIVE_VALS in addition to, not instead of, calling setsockopt. I verified that the keep alive packets are not sent when the Windows node has ZMQ_TCP_KEEPALIVE by tcpdumping on a Linux system that was connected to the Windows node as a ZMQ subscriber.

These changes are still good for Linux systems. These notes just document how Windows is not affected by this PR because of a bug in the ZMQ library.

Additonally, I verified that setting ZMQ_TCP_KEEPALIVE on the subscriber side does work to keep the connections alive and working. Doing that is an alternative to this PR (which sets ZMQ_TCP_KEEPALIVE on the publisher side). The goal is to keep packets going through whatever intermediary has the idle timeout that silently drops the connection. Therefore, the keep alive can be set on either side of the TCP connection to be effective.

Helpful related links:

Testing note: the following commands were useful to setup the subscriber side Linux system so that it’s firewall would silently drop the connection after 30 seconds of being idle (when ZMQ_TCP_KEEPALIVE was not set [or having the desired effect]).

0# set the keep alive packets to go out every second if the sockets are configured to use keep alive
1sudo sysctl -w net.ipv4.tcp_keepalive_time=1
2sudo sysctl -w net.ipv4.tcp_keepalive_intvl=1
3# setup iptables to drop non-loopback packets that are not in conntrack as being established
4sudo /sbin/iptables -A INPUT -i lo -j ACCEPT
5sudo /sbin/iptables -A INPUT -m conntrack --ctstate ESTABLISHED -j ACCEPT
6sudo /sbin/iptables -P INPUT DROP
7# make conntrack forget idle TCP connections after 30 seconds of inactivity
8# be sure to run the next command AFTER configuring conntrack in iptables, otherwise the files may not already exist under /proc/sys.
9sudo sysctl -w net.netfilter.nf_conntrack_tcp_timeout_established=30

adamjonas commented at 5:16 pm on June 17, 2020: member

Just to summarize for those looking to review - as of c276df775914e4e42993c76e172ef159e3b830d4 there are 3 tACKs (n-thumann, Haaroon, and dlogemann), 1 “looks good to me” (laanwj) with no NACKs or any show-stopping concerns raised.

adaminsky commented at 0:29 am on August 8, 2020: contributor

I also cherry picked the changes into the current master branch and tested on macOS 10.15.5. I started bitcoind on regtest and used python3 contrib/zmq/zmq_sub.py to subscribe. I changed the keepalive time to one second with sudo sysctl -w net.inet.tcp.keepidle=1000 and was able to observe with tcpdump the keepalive packets every second.

Also, @adamjonas, I think you mean there were 3 tACKs, this seems like it has been pretty well tested.

adamjonas commented at 1:14 pm on August 8, 2020: member

@adaminsky updated my previous comment to correct my error. Thanks.

instagibbs commented at 2:28 am on September 2, 2020: member

I think this is ready for merge, unless I’m missing some controversy.

jonasschnelli approved

jonasschnelli commented at 7:08 am on September 2, 2020: contributor

utACK c276df775914e4e42993c76e172ef159e3b830d4

jonasschnelli merged this on Sep 2, 2020

jonasschnelli closed this on Sep 2, 2020

sidhujag referenced this in commit 73c1218670 on Sep 3, 2020

mruddy deleted the branch on Sep 12, 2020

PastaPastaPasta referenced this in commit 9d7f30252c on Sep 17, 2021

PastaPastaPasta referenced this in commit 516cfb54c5 on Sep 24, 2021

kittywhiskers referenced this in commit 4bb73be509 on Oct 12, 2021

Fabcien referenced this in commit c0200506cb on Oct 13, 2021

DrahtBot locked this on Feb 15, 2022

zmq: enable tcp keepalive #14687