RPC: add client startuptime to RPC getinfo #1352

Diapolo commented at 8:49 AM on May 18, 2012: none

add GetClientStartupTime() to util.cpp/.h, which returns a static const int64 from util.cpp
change indentation in getininfo() function, to match all lines (better readability)
remove some spaces in comments

Perhaps it makes sense to place it somewhere else in the getinfo, but I didn't want to break things that perhaps rely on the current ordering.

gavinandresen commented at 3:25 PM on May 18, 2012: contributor

How did you test this?

Diapolo commented at 4:32 PM on May 18, 2012: none

With an own build and the RPC console which is part of the current master. Anything badly wrong with the code or what's your intention to ask?

gavinandresen commented at 5:07 PM on May 18, 2012: contributor

I'm looking for a simple test plan, something like:

Note the time and immediately run bitcoin

Wait two minutes. Call getinfo

EXPECT: client startup time reported is the time noted immediately before starting bitcoin (or within a second or two)

If I'm reading your code correctly, that simple test will fail, which is why I'm asking how you tested.

Diapolo commented at 5:18 PM on May 18, 2012: none

Now I get it, you are right ... that is not a problem for Bitcoin-Qt as GetClientStartupTime() is called on init, which is not true for bitcoind. So for this to work we (I) need a place to be allowed to call GetClientStartupTime(). What is the best place to do this? I did not want to use a global, that's why I chose the current code.

add GetClientStartupTime() to util.cpp/.h / add startuptime to RPC getinfo / remove some spaces in comments 753e9bc8db

in src/qt/clientmodel.cpp:None in b733b11940 outdated

   4 | @@ -5,7 +5,7 @@
   5 |  #include "transactiontablemodel.h"
   6 |  
   7 |  #include "main.h"
   8 | -static const int64 nClientStartupTime = GetTime();

luke-jr commented at 7:57 PM on May 18, 2012:

IMO this was a much better/simpler way to do it... ;)

Diapolo commented at 9:23 PM on May 18, 2012:

At least easier, but I hate globals ... sorry. Can you give a suggestion, where to place the initial call to GetClientStartupTime() :)?

luke-jr commented at 9:30 PM on May 18, 2012:

It's global either way. Global functions aren't much better than global variables...

A function with attribute((constructor)) can provide the same behaviour, but it's a GCC extension (non-standard) and using a global variable is much cleaner.

Diapolo commented at 9:33 PM on May 18, 2012:

I had a bad feeling about leaving another global in the bunch of globals that are used in the code. Should I come over this feeling?

Edit: And clientmodel.cpp would be the wrong place if I want to use that in RPC.

luke-jr commented at 9:52 PM on May 18, 2012:

I think for this, a global is the best solution. Perhaps move it to util.cpp?

Diapolo commented at 10:28 PM on May 18, 2012: none

Changed back to a global, which removes the need to explicitly initialize the static const int64. This needs to get tested for bitcoind, as I can't compile that! Tested with Bitcoin-Qt and there it works. The startuptime in getinfo via RPC console doesn't change.

in src/util.cpp:None in 753e9bc8db

1102 | @@ -1101,3 +1103,8 @@ string FormatFullVersion()
1103 |      return fs::path("");
1104 |  }
1105 |  #endif
1106 | +
1107 | +int64 GetClientStartupTime()

luke-jr commented at 11:12 PM on May 18, 2012:

Still no need for a function...

Diapolo commented at 11:19 PM on May 18, 2012:

How can I access nClientStartupTime in other cpp's then? I was not succesful with extern or placing nClientStartupTime in util.h as it was above GetTime(), which was missing then ;).

luke-jr commented at 11:47 PM on May 18, 2012:

Remove "static" in this file, and add to util.h:

extern const int64 nClientStartupTime;

Diapolo commented at 9:00 AM on May 19, 2012:

Would you mind testing this for bitcoind then, so we can re-open? Will push an update and remove the static + unneeded function.

gavinandresen closed this on May 19, 2012

gavinandresen commented at 2:09 AM on May 19, 2012: contributor

Closing this until it is tested with bitcoind.

Diapolo commented at 9:13 AM on May 19, 2012: none

Updated and hoping someone can test this :). @luke-jr See: https://github.com/Diapolo/bitcoin/commit/e9558a2bd022823e232b23443ed2a124ed3715b3

I don't know, why the last update is not showing up here ... weird.

luke-jr commented at 12:30 PM on May 19, 2012: member

Note the time and immediately run bitcoind
Wait until debug.log shows it has finished starting
Wait two more minutes. Call getinfo EXPECT: client startup time reported is the time noted immediately before starting bitcoin (or within a second or two)

RESULT: Works correctly with bitcoind and Bitcoin-Qt.

Gavin, please reopen so GitHub updates the pullreq.

gavinandresen commented at 12:42 PM on May 19, 2012: contributor

NACK. Process startup time is available from the OS, no need for this functionality: http://linuxcommando.blogspot.com/2008/09/how-to-get-process-start-date-and-time.html

(don't know about Windows, but I'm sure it has something similar)

luke-jr commented at 4:44 PM on May 19, 2012: member

Gavin, this isn't adding new functionality, it's just making bitcoind on par with a feature already available in Bitcoin-Qt. Process startup time is only available from the OS if someone is on the local system. bitcoind still supports binding external IPs for LAN use.

Diapolo commented at 10:18 PM on May 19, 2012: none

I think it's rather bad to use OS-specific code for such a small thing like this is. The code is portable and already in (only in another .cpp file). Like luke-jr said...

Diapolo commented at 1:09 PM on May 20, 2012: none

Rebased to be mergable again! @gavinandresen What about re-open and accepting this or do you still prefer a non-portable approach?

suprnurd referenced this in commit e3288a7e14 on Dec 5, 2017

lateminer referenced this in commit 8f1e2cf841 on Jan 22, 2019

lateminer referenced this in commit fe60594c4a on May 6, 2020

DrahtBot locked this on Sep 8, 2021