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
reddink referenced this in commit 95ca652195 on May 27, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

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

Replace 86400 with 246060 to make it more consistent #4724