[Net] Clarity TIMEOUT_INTERVAL constant meaning. #9716

pull pstratem wants to merge 2 commits into bitcoin:master from pstratem:2017-02-07-ping-timeout-interval changing 2 files +4 −4

pstratem commented at 2:49 AM on February 8, 2017: contributor

No description provided.
Rename TIMEOUT_INTERVAL to PING_TIMEOUT_INTERVAL for clarity. 080ff0aa15
Define PING_TIMEOUT_INTERVAL as a multiple of TIMEOUT_INTERVAL. 18345647f5
pstratem force-pushed on Feb 8, 2017
fanquake added the label P2P on Feb 8, 2017

in src/net.h:None in 18345647f5

  46 | @@ -47,7 +47,7 @@ namespace boost {
  47 |  /** Time between pings automatically sent out for latency probing and keepalive (in seconds). */
  48 |  static const int PING_INTERVAL = 2 * 60;
  49 |  /** Time after which to disconnect, after waiting for a ping response (or inactivity). */
  50 | -static const int TIMEOUT_INTERVAL = 20 * 60;
  51 | +static const int PING_TIMEOUT_INTERVAL = PING_INTERVAL * 5;

TheBlueMatt commented at 4:08 PM on February 8, 2017:

Why change the number?

MarcoFalke commented at 6:15 PM on February 8, 2017: member

Looks like time is denoted in int64_t elsewhere. When changing the name, you might as well make the type static constexpr int64_t, so we don't need to rely on implicit promotion whenever the constant is used.

On Wed, Feb 8, 2017 at 5:08 PM, Matt Corallo notifications@github.com wrote:

@TheBlueMatt commented on this pull request.

In src/net.h:

@@ -47,7 +47,7 @@ namespace boost { /** Time between pings automatically sent out for latency probing and keepalive (in seconds). */ static const int PING_INTERVAL = 2 * 60; /** Time after which to disconnect, after waiting for a ping response (or inactivity). */ -static const int TIMEOUT_INTERVAL = 20 * 60; +static const int PING_TIMEOUT_INTERVAL = PING_INTERVAL * 5;

Why change the number?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
sipa commented at 9:44 PM on February 8, 2017: member

I'm not convinced the constant rename helps. From the code it looks like a general timeout for received messages, not just a ping timeout?
pstratem commented at 1:47 AM on February 14, 2017: contributor

@sipa it's a general timeout for having sent anything to the other peer, however the only thing guaranteeing we send peers a message at regular intervals is ping, it's critical that TIMEOUT_INTERVAL be larger than PING_INTERVAL because of this, but it's not obvious where the constants are defined from their names
laanwj commented at 12:48 PM on November 9, 2017: member

There seems no agreement on doing this, and the issue has been dormant for months, so closing.
laanwj closed this on Nov 9, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels