Ping automatically every 2 minutes (unconditionally), instead of of after 30 minutes of no sending, for latency measurement and keep-alive. Also, disconnect if no reply arrives within 5 minutes, instead of after 90 minutes of inactivity.
This should make detection of stalled connections much faster.
Inverted the logic, so it now becomes a ping if nothing has been received for a while, rather than sent.
Tested that it indeed detects broken connections within one minute.
Change looks good to me, although it will have the effect of disconnecting any nodes that don’t respond to pings with pongs (or some other message). As pong messages were added in protocol version 60000 it would effectively EOL clients older than that and this may deserve an announcement somewhere.
However bitcoinj clients will be fine with it.
So, this causes nodes to impose an upper bound on their peers response latency. Effectively, someone who will take a minute to respond can’t participate in the network at all. This may adversely impact some anonymity protocols which cause high latency— for many kinds of Bitcoin usage the latency isn’t all that critical. I’d suggest that such users should be using an alternative transport, except the bitcoin p2p protocol is already reasonably well designed for high latency usage (at least its highly asynchronous).
Some of the other side effects of timeouts is that they can reduce link stability during a DOS attack, and they can preclude some kinds of node high availability schemes which might cause tens of seconds of unresponsiveness (e.g. during a state resync or hardware migration). Connection slot filling attacks are more effective if you can overload a peer temporarily and make them drop all their good connections.
Some other protocols, like BGP, negotiate the heart-beating— 30, 90 being a common set of parameters in that case— to address the problem of potentially imposing too fast a response requirement. But since our network protocol is mostly stateless (though— it has become less so with bloom-filtering, and never was completely: e.g. inv caching) there is less of an issue as restarts aren’t so bad.
I’d instead prefer a longer timeout and separately having peer rotation code that periodically slays the least recently active node from node out of the set which been heard from in >60 seconds. E.g. imposing the shorter timeout but only when we could potentially better use the slot.
TCP’s design targets a maximum segment lifetime of 2 minutes. Many operating systems have an SO_KEEPALIVE timeout of around 10 minutes. Somewhere in that range
It would be easy to adjust the timeout for onion addresses, but really, I’m not sure we want nodes in the network that can’t respond within a minute. Slow nodes can have terrible effects on the user experience for SPV wallets. They will automatically drop to the bottom of the preference list in bitcoinj and not be used for much except observing broadcasts, for that reason.
That said, just dropping the slowest peer when we run out of slots on the bitcoind side indeed sounds reasonable.
There may be a bug in this code; will investigate later.
EDIT: fixed.
ACK
Optional nit: “5 * 60” is more self-documenting than “300”, and the previous code used the “M * 60” notation. All modern compilers will automatically convert the more human-readable M*60 at compile time, so there is no added runtime cost.
240@@ -237,11 +241,11 @@ class CNode
241 std::multimap<int64, CInv> mapAskFor;
242
243 // Ping time measurement
244- uint64 nPingNonceSent;
245- int64 nPingUsecStart;
246- int64 nPingUsecTime;
247- bool fPingQueued;
248-
249+ uint64 nPingNonceSent; // The pong reply we're expecting, or 0 if no pong expected.
Rebased, and made some changes (better reporting, and don’t enforce 5-minute receive timeout on pre-BIP31 peers).
I’m testing it again myself.
Seems the majority of timeouts detected are ping timeouts from getaddr.bitnodes.io (which apparently only recently implemented ping/pong - though probably not yet deployed).
Apart from that I do quite some others, from various clients and IPs, every ~10 minutes on average the past hours. These look like genuine connection problems.
2014-05-11 10:27:53 Sending ping to [2a01:4f8:202:81b1::2]:48980 /getaddr.bitnodes.io:0.1/
No pong afterwards (while I do receive pongs from other nodes).
For reference: the problem with bitnodes.io was lingering connections from a crawler that didn’t support BIP31 (but advertized a protocol version that did), which was fixed on their side now.
I’m seeing occassional disconnects because of the new ping timeouts, but I believe these are correct.
Any objections to merging?
importprivkey rpc command can lock cs_main for many minutes during a rescan. If during that time a network message arrives which also requires a lock on cs_main (for example a harmless inv), this locks the network thread until the rescan is complete. This leaves the peer unable to answer pings for more than 10 minutes.
@wumpus TCP itself will fail over long before that if the network itself is delayed. A node thats holding up our connection but not responding for >5 minutes is really broken as far as we should care. It can reconnect when it can start responding again. (and yes, in some cases bitocoind is ‘broken’, but thats okay… reconnecting is fairly cheap).
The flip side of a long timeout is that we’ll leave connections up to busted peers— keeping ourselves offline (or our resources consumed) for up to the timeout— when we could otherwise have useful connections up.
Maybe it would make sense to have a conservative upper limit for now, I certantly wouldn’t want to delay pulling this further to debate the timeout. I’m happy with anything 5 minutes or longer but would prefer closer to 5 minutes. Future peer rotation could prioritize punting long idle connection, which would address the problem that you might sit partitioned from the network for many minutes with no working connections.
... instead of after 30 minutes of no sending, for latency measurement
and keep-alive. Also, disconnect if no reply arrives within 20 minutes,
instead of 90 of inactivity (for peers supporting the 'pong' message).
Just tested on mainnet:
The problem is the node is busy sending us blocks, so does not respond to the ping. I think we should delay the ping when blocks are in flight.
