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

zmq: enable tcp keepalive #14687

Merged
merged 1 commit into from Sep 2, 2020
Merged

zmq: enable tcp keepalive #14687

merged 1 commit into from Sep 2, 2020

Conversation

@mruddy
Copy link
Contributor

@mruddy mruddy commented Nov 8, 2018

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 &

@bitcoin bitcoin deleted a comment from DrahtBot Nov 8, 2018
doc/zmq.md Outdated
sudo sysctl -w net.ipv4.tcp_keepalive_time=600

Setting the keepalive values appropriately for your operating environment may
improve connectiviy in situations where long-lived connections are silently

This comment has been minimized.

@practicalswift

practicalswift Nov 9, 2018
Contributor

"connectivity" :-)

This comment has been minimized.

@mruddy

mruddy Nov 9, 2018
Author Contributor

fixed, thanks :-)

@mruddy mruddy force-pushed the mruddy:zmq-keep-alive branch Nov 9, 2018
@laanwj
Copy link
Member

@laanwj laanwj commented Nov 12, 2018

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

Copy link
Member

@promag promag left a comment

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

src/zmq/zmqpublishnotifier.cpp Outdated
@@ -86,6 +86,15 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext)
return false;
}

const int so_keepalive_option {1};
rc = zmq_setsockopt(psocket, ZMQ_TCP_KEEPALIVE, &so_keepalive_option, sizeof(so_keepalive_option));
if (rc != 0)

This comment has been minimized.

@promag

promag Nov 15, 2018
Member

nit, { here.

This comment has been minimized.

@mruddy

mruddy Nov 15, 2018
Author Contributor

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 :)

For example, when running on GNU/Linux, one might use the following
to lower the keepalive setting to 10 minutes:

sudo sysctl -w net.ipv4.tcp_keepalive_time=600

This comment has been minimized.

@promag

promag Nov 15, 2018
Member

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

This comment has been minimized.

@mruddy

mruddy Nov 15, 2018
Author Contributor

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?

@mruddy
Copy link
Contributor Author

@mruddy mruddy commented Nov 15, 2018

is it possible to enable keepalive globally?

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

@promag
Copy link
Member

@promag promag commented Nov 15, 2018

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

@mruddy mruddy force-pushed the mruddy:zmq-keep-alive branch to c276df7 Nov 15, 2018
rc = zmq_setsockopt(psocket, ZMQ_TCP_KEEPALIVE, &so_keepalive_option, sizeof(so_keepalive_option));
if (rc != 0) {
zmqError("Failed to set SO_KEEPALIVE");
zmq_close(psocket);

This comment has been minimized.

@luke-jr

luke-jr Dec 20, 2018
Member

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

This comment has been minimized.

@laanwj

laanwj Jan 8, 2019
Member

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.

This comment has been minimized.

@jgarzik

jgarzik Jan 8, 2019
Contributor

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.

This comment has been minimized.

@luke-jr

luke-jr Jan 8, 2019
Member

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

This comment has been minimized.

@jgarzik

jgarzik Jan 8, 2019
Contributor

zmq will return 0 (success) on systems without SO_KEEPALIVE

@dlogemann
Copy link

@dlogemann dlogemann commented Mar 13, 2019

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
Copy link
Contributor Author

@mruddy mruddy commented Mar 13, 2019

@dlogemann thanks for testing!

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

@dannybabbev
Copy link

@dannybabbev dannybabbev commented May 1, 2019

Hi, when will this PR be merged?

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented May 2, 2019

Needs more review.

@DrahtBot DrahtBot closed this Aug 16, 2019
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Aug 16, 2019

The last travis run for this pull request was 273 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
@DrahtBot DrahtBot reopened this Aug 16, 2019
@Haaroon
Copy link

@Haaroon Haaroon commented Dec 3, 2019

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

@laanwj
Copy link
Member

@laanwj laanwj commented Dec 3, 2019

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
Copy link

@Haaroon Haaroon commented Dec 4, 2019

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
Copy link
Contributor

@n-thumann n-thumann commented Apr 23, 2020

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
Copy link
Contributor Author

@mruddy mruddy commented May 5, 2020

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

sha256sum Win10_1909_English_x64.iso
01bf1eb643f7e50d0438f4f74fb91468d35cde2c82b07abc1390d47fc6a356be 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]).

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

@adamjonas adamjonas commented Jun 17, 2020

Just to summarize for those looking to review - as of c276df7 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
Copy link
Contributor

@adaminsky adaminsky commented Aug 8, 2020

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
Copy link
Member

@adamjonas adamjonas commented Aug 8, 2020

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

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Sep 2, 2020

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

Copy link
Member

@jonasschnelli jonasschnelli left a comment

utACK c276df7

@jonasschnelli jonasschnelli merged commit 3a3e21d into bitcoin:master Sep 2, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 3, 2020
c276df7 zmq: enable tcp keepalive (mruddy)

Pull request description:

  This addresses bitcoin#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 &`

ACKs for top commit:
  adamjonas:
    Just to summarize for those looking to review - as of c276df7 there are 3 tACKs (n-thumann, Haaroon, and dlogemann), 1 "looks good to me" (laanwj) with no NACKs or any show-stopping concerns raised.
  jonasschnelli:
    utACK c276df7

Tree-SHA512: b884c2c9814e97e666546a7188c48f9de9541499a11a934bd48dd16169a900c900fa519feb3b1cb7e9915fc7539aac2829c7806b5937b4e1409b4805f3ef6cd1
@mruddy mruddy deleted the mruddy:zmq-keep-alive branch Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet