add -timeout default as constant and use them #4979

Diapolo commented at 7:02 AM on September 25, 2014: none

update help message text
simplify code in init to check for -timeout

in src/netbase.h:None in 4ff78e7e96 outdated

  18 | @@ -19,6 +19,11 @@
  19 |  extern int nConnectTimeout;
  20 |  extern bool fNameLookup;
  21 |  
  22 | +/** -connect default */
  23 | +static const int DEFAULT_CONNECT_TIMEOUT = 5000;
  24 | +/** maximum allowed value for -connect */

laanwj commented at 7:08 AM on September 25, 2014:

maximum value allowed for -connect?

Diapolo commented at 7:20 AM on September 25, 2014:

-timeout indeed, thanks!

BitcoinPullTester commented at 7:56 AM on September 25, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4979_15d5ce78c5587c4c5f872860ebfd066a0d7f2a9c/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Diapolo commented at 12:31 PM on October 1, 2014: none

@sipa @laanwj Any ACK here?

TheBlueMatt commented at 5:45 PM on October 1, 2014: member

You should provide some reasoning why this is actually useful.

Diapolo commented at 6:39 PM on October 1, 2014: none

IMHO a fully detailed description of the help message alone and the use of not single "magic" numbers (-timeout is perhaps not the best example ^^) in the code would be worth it for ALL parameters we have.

TheBlueMatt commented at 8:55 PM on October 1, 2014: member

That didnt really answer the question. Why do you want to limit the user's minimum and maximum connection timeout?

sipa commented at 2:06 AM on October 2, 2014: member

utACK.

This doesn't change any behavior, just introduced constants.

TheBlueMatt commented at 2:15 AM on October 2, 2014: member

OK. Better question...why do we have a max connect timeout?

laanwj commented at 7:29 AM on October 2, 2014: member

@TheBlueMatt good point. I think the max connect timeout should go, if a user wants to wait $maxint milis, that's up to them.

Diapolo commented at 8:39 AM on October 2, 2014: none

It's no problem to implement that via this pull.

Diapolo commented at 9:18 AM on October 2, 2014: none

Updated, I get 2147483647 now, is that correct?

in src/init.cpp:None in 45b10f8d7f outdated

 642 | @@ -643,8 +643,8 @@ bool AppInit2(boost::thread_group& threadGroup)
 643 |  
 644 |      if (mapArgs.count("-timeout"))
 645 |      {
 646 | -        int nNewTimeout = GetArg("-timeout", 5000);
 647 | -        if (nNewTimeout > 0 && nNewTimeout < 600000)
 648 | +        int nNewTimeout = GetArg("-timeout", DEFAULT_CONNECT_TIMEOUT);
 649 | +        if (nNewTimeout > 0 && nNewTimeout < MAX_CONNECT_TIMEOUT)

Diapolo commented at 9:19 AM on October 2, 2014:

I guess it should be <= now ;)?

laanwj commented at 9:47 AM on October 2, 2014: member

@Diapolo please just remove the check for a maximum, not update the constant

Diapolo commented at 9:49 AM on October 2, 2014: none

@laanwj What would happen then if someone enters 2147483648 for example, which is >maxint!?

laanwj commented at 9:51 AM on October 2, 2014: member

That should be handled by the number parser, not by an explicit check. Remember that checking >=int_max is a no-op as they can't contain a larger value than that.

add -timeout default as constant and use them

- update help message text
- simplify code in init to check for -timeout

de10efd154

Diapolo renamed this:
~~add -timeout defaults as constants and use them~~
add -timeout default as constant and use them
on Oct 6, 2014

Diapolo commented at 11:15 AM on October 6, 2014: none

@laanwj Removed the max check and also simplified the code in init. @TheBlueMatt Thanks for your suggestion for this.

in src/init.cpp:None in de10efd154

 259 | @@ -260,7 +260,7 @@ std::string HelpMessage(HelpMessageMode mode)
 260 |      strUsage += "  -port=<port>           " + _("Listen for connections on <port> (default: 8333 or testnet: 18333)") + "\n";
 261 |      strUsage += "  -proxy=<ip:port>       " + _("Connect through SOCKS5 proxy") + "\n";
 262 |      strUsage += "  -seednode=<ip>         " + _("Connect to a node to retrieve peer addresses, and disconnect") + "\n";
 263 | -    strUsage += "  -timeout=<n>           " + _("Specify connection timeout in milliseconds (default: 5000)") + "\n";
 264 | +    strUsage += "  -timeout=<n>           " + strprintf(_("Specify connection timeout in milliseconds (minimum: 1, default: %d)"), DEFAULT_CONNECT_TIMEOUT) + "\n";

TheBlueMatt commented at 7:24 AM on October 7, 2014:

Is there/should there be a minimum?

laanwj commented at 8:04 AM on October 7, 2014:

Well the specific value of 0 for timeouts is meaningless. One has to give the physical world some time to respond. So are negative values. Not sure how much sense it makes to mention it though. Right now, the behaviour is that <=0 will select the default (it could also just fail).

TheBlueMatt commented at 5:46 PM on October 7, 2014: member

utACK

laanwj merged this on Oct 8, 2014

laanwj closed this on Oct 8, 2014

laanwj referenced this in commit 6860a55ea0 on Oct 8, 2014

Diapolo deleted the branch on Oct 8, 2014

MarcoFalke locked this on Sep 8, 2021