Replace 86400 with 246060 to make it more consistent #4724

pull ghost wants to merge 1 commits into bitcoin:master from changing 1 files +2 −2

ghost commented at 7:35 PM on August 18, 2014: none

Constants such as 86400 are regularly used, so 600 seconds as 10 minutes makes sense to me to use here.
jgarzik commented at 8:48 PM on August 18, 2014: contributor

This is moving in the direction of less readability, less information.

The compiler handles such calculations at compile time, so there is no need to put the number in a form less friendly to humans.
sipa commented at 8:50 PM on August 18, 2014: member

I'd rather move in the opposite direction too.
ghost commented at 9:04 PM on August 18, 2014: none

Thanks for checking this out. I personally disagree. I think that anyone reading the code knows that there are 86400 seconds in a day, and that 600 seconds is 10 minutes.

Would you rather see constants for 86400, like DAY, and 10MIN for 600? Or do you prefer 60 * 60 * 24?
jgarzik commented at 9:12 PM on August 18, 2014: contributor
You must think in the context of changes over time, and how a change looks in a diff, not just the constant in isolation. Changing 24 to 20 hours is much more readable using
```
   - 24 * 60 * 60
   + 20 * 60 * 60
```
than
```
   - 86400
   + 72000
```
This notation is intentional. It creates self-documenting, maintainable code, that permits easier review of the time units being changed. Subtle, but IMO this is important to security critical code.
ghost commented at 9:47 PM on August 18, 2014: none

Thank you, that's a really good point I didn't think about. I'll see if I can refactor this.
Make seconds more consistent in addrman
Replaced 86400 with 24*60*60
8a8f034757
unknown renamed this:
~~Replace 10*60 with 600 to make it more consistent~~
Replace 86400 with 24*60*60 to make it more consistent
on Aug 18, 2014
BitcoinPullTester commented at 11:05 PM on August 18, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4724_8a8f03475753b4a17eb3eaafc0beee5169e832cb/ 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.
in src/addrman.cpp:None in 8a8f034757
```
  44 | @@ -45,13 +45,13 @@ bool CAddrInfo::IsTerrible(int64_t nNow) const
  45 |      if (nTime > nNow + 10*60) // came in a flying DeLorean
  46 |          return true;
  47 |  
  48 | -    if (nTime==0 || nNow-nTime > ADDRMAN_HORIZON_DAYS*86400) // not seen in over a month
  49 | +    if (nTime==0 || nNow-nTime > ADDRMAN_HORIZON_DAYS*24*60*60) // not seen in over a month
```
laanwj commented at 9:41 AM on August 19, 2014:

We should also remove the hardcoded values from the comment. Someone updating the ADDRMAN_HORIZON_DAYS will likely forget to update the comment here. (same goes for "tried three times and never a success" and "10 successive failures in the last week" below. Maybe even remove the comments as the code is self-documenting.
laanwj added the label Improvement on Aug 19, 2014
laanwj commented at 3:06 AM on August 30, 2014: member

@sega01 can you do the trivial comment change I suggested above, then we can merge this
laanwj referenced this in commit f79323b0dd on Sep 5, 2014
laanwj commented at 11:50 AM on September 5, 2014: member

Merged via f79323b
laanwj closed this on Sep 5, 2014
laanwj cross-referenced this on Nov 25, 2014 from issue Incorrect use of gcc -frandom-seed ? by gavinandresen
reddink referenced this in commit 95ca652195 on May 27, 2020
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#2290 Incorrect use of gcc -frandom-seed ?

Replace 86400 with 24*60*60 to make it more consistent #4724

Replace 86400 with 246060 to make it more consistent #4724