[RPC] Add an uptime command that displays the amount of time (in seconds) bitcoind has been running

rvelhote commented at 6:44 pm on May 14, 2017: contributor

Hello everyone,

I am working on a tool that displays information about a bitcoin node using RPC. One of the parameters I would like to include is the uptime of the bitcoin daemon. Currently, as far as I can tell, there is no way of obtaining such information.

After thinking a bit on how to perform this task, I concluded that the easiest way to do it is to check the modification time of the bitcoind.pid file. This has the disadvantage that it does not work in Windows because bitcoind.pid is not created.

I propose the following solution in this PR and submit it for your review and opinion concerning this new RPC command before I work further on this topic. Do you think this new command makes sense? Please consider it a WIP/PoC as it does not have any error checking or tests.

Here is a sample return from the RPC command:

0$ ./bitcoin-cli uptime  
1{
2  "uptime": 696
3}

and the help function:

0$bitcoin-cli help uptime  
1uptime
2
3Display the uptime of the Bitcoin server.

Best regards, Ricardo Velhote

fanquake added the label RPC/REST/ZMQ on May 15, 2017

laanwj commented at 5:03 am on May 15, 2017: member

After thinking a bit on how to perform this task, I concluded that the easiest way to do it is to check the modification time of the bitcoind.pid file.

There is no need to look at the timestamp of any files - the server could simply remember its own startup timestamp and use that as a reference. This would work just as well on windows.

rvelhote commented at 7:48 am on May 15, 2017: contributor

Thank you for replying Wladimir. You are correct of course. Checking the PID file was the most simple and with the least changes to the codebase I could think of - while still being useful (I am not familiar with the Bitcoin core and have not worked with C++ since 2010).

So, if you think this command is useful for inclusion in the core, I could work on improving it and then squash the commits in the end.

Should this information be its own command or should it be included in another existing command - if so, which one?

Thank you, Ricardo Velhote

jonasschnelli commented at 7:50 am on May 15, 2017: contributor

It seems to be useful. Concept ACK.

Agree with @laanwj. Just set a variable during startup with GetTime() (now) and calculate the delta to “now()” when someone want’s to know the current uptime.

paveljanik commented at 9:00 am on May 15, 2017: contributor

Concept ACK

The other way could be adding starttime to getinfo RPC. But I prefer your solution, separate simple approach, new RPC. Please also add a test case for this.

laanwj commented at 10:40 am on May 15, 2017: member

The other way could be adding starttime to getinfo RPC. But I prefer your solution, separate simple approach, new RPC. Please also add a test case for this.

A gentle reminder that getinfo should not be extended, see https://github.com/bitcoin/bitcoin/blob/master/src/rpc/misc.cpp#L34:

 0/**
 1 * [@note](/bitcoin-bitcoin/contributor/note/) Do not add or change anything in the information returned by this
 2 * method. `getinfo` exists for backwards-compatibility only. It combines
 3 * information from wildly different sources in the program, which is a mess,
 4 * and is thus planned to be deprecated eventually.
 5 *
 6 * Based on the source of the information, new information should be added to:
 7 * - `getblockchaininfo`,
 8 * - `getnetworkinfo` or
 9 * - `getwalletinfo`
10 *
11 * Or alternatively, create a specific query method for the information.
12 **

One of the other get*info would be an option but I don’t see where it’d clearly fit in. So agree a separate call is OK.

achow101 commented at 3:24 pm on May 15, 2017: member

There’s already a variable in the GUI for getting the startup time: https://github.com/bitcoin/bitcoin/blob/86ea3c2ff247bb2ba0fb50013c8ecdbaf8a9fe8f/src/qt/clientmodel.cpp#L29

You could probably just move it so that it can be used for both the GUI and for this RPC call.

rvelhote commented at 7:55 am on May 16, 2017: contributor

Thank you for your feedback. I went ahead and made some modifications according to your suggestions.

I have pushed a new commit that removes nClientStartupTime from being specific to the QT interface and moved it to util.h where it can be used by both the QT interface and the RPC server.

I called it nStartupTime instead of nClientStartupTime because now it related to both the client and the daemon. I also decided on util.h instead of init.h. The reason is that the startup time is more of an utility/informational feature rather than a core initialization feature.

I am not sure of the naming conventions. Should I use g_nStartupTime to symbolize that it’s a global variable or is that used for specific objects like g_connman?

I have build the QT client (QT5 in my case) and the daemon and from my testing every is working well. I have also started the QT client with the -server flag and both are returning the correct value. The QT client displays the startup date in an ISO format (Debug Window » Information) and the RPC server displays the amount of seconds that have passed since that startup date.

I will add a functional test to the RPC interface for this command. I’m not sure how the testing is done for the QT client.

in src/util.h:32 in 389541e8e8 outdated

28@@ -29,6 +29,9 @@
29 #include <boost/signals2/signal.hpp>
30 #include <boost/thread/exceptions.hpp>
31 
32+// Bitcoin startup time

paveljanik commented at 10:25 am on May 16, 2017:

// Application startup time (used for uptime calculation)

in src/rpc/server.cpp:270 in 389541e8e8 outdated

265+                "uptime\n"
266+                        "\nDisplay the uptime of the Bitcoin server.");
267+
268+    int64_t uptime = GetTime() - nStartupTime;
269+
270+    if (uptime < 0)

paveljanik commented at 10:30 am on May 16, 2017:

Do we need this at all?

in src/rpc/server.cpp:274 in 389541e8e8 outdated

269+
270+    if (uptime < 0)
271+    {
272+        throw std::runtime_error(
273+                "stop\n"
274+                        "\nUptime value for the Bitcoin server is a negative value.");

paveljanik commented at 10:35 am on May 16, 2017:

Started in the future!? ;-)

rvelhote commented at 10:44 am on May 16, 2017:

I imagined that for some weird reason the clock would change to before the start and the user would get a negative value. Maybe I’m over-thinking this? ;)

paveljanik commented at 10:52 am on May 16, 2017:

Yup, I do not have a problem with this. Just return negative value and do not crash.

in src/rpc/server.cpp:266 in 389541e8e8 outdated

257@@ -258,6 +258,27 @@ UniValue stop(const JSONRPCRequest& jsonRequest)
258     return "Bitcoin server stopping";
259 }
260 
261+UniValue uptime(const JSONRPCRequest& jsonRequest)
262+{
263+    if (jsonRequest.fHelp || jsonRequest.params.size() > 1)
264+        throw std::runtime_error(
265+                "uptime\n"
266+                        "\nDisplay the uptime of the Bitcoin server.");

paveljanik commented at 10:36 am on May 16, 2017:

Let’s document output as Result: as other RPCs. Add unit specification (seconds).

rvelhote commented at 11:24 pm on May 16, 2017: contributor

I have made a new commit with some changes suggested by Pavel.

While searching for examples of documentation I noticed getconnectioncount which only returns a single value (the node count). Previously the uptime command was returning an object with an uptime key so I followed the example of getconnectioncount and changed the return value to a single integer that represents the amount of seconds instead of an object.

Here is a sample of the documentation when running bitcoin-cli help uptime

 0$ bitcoin-cli help uptime
 1uptime
 2
 3Returns the total uptime of the Bitcoin server.
 4
 5Result:
 6ttt        (numeric) The number of seconds that the Bitcoin server has been running
 7
 8Examples:
 9> bitcoin-cli uptime 
10> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "uptime", "params": [] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/

I used the ttt designation because I’ve seen it in other areas to indicate a time-based value (e.g. getpeerinfo).

rvelhote commented at 9:58 pm on May 17, 2017: contributor

I’ve added a functional test to check for the uptime value. In this test I’ve placed a time.sleep before calling the uptime command. The test then checks if the current uptime value reported by the RPC command is greater or equal than the value used in time.sleep.

As I’ve noted in the test comment:

Depending on how fast the setup executes, the RPC server starts and this test executes the check for the uptime value might not deterministic. At the very least we will check if the reported uptime is the amount of seconds we waited.

in test/functional/server.py:32 in 2a57561973 outdated

27+        wait_time = 6
28+
29+        # Depending on how fast the setup executes, the RPC server starts and this test
30+        # executes the check for the uptime value might not deterministic. At the very
31+        # least we will check if the reported uptime is the amount of seconds we waited.
32+        time.sleep(wait_time)

paveljanik commented at 6:23 am on May 18, 2017:

You can use setmocktime to not really sleep/slow down tests for 6 seconds.

rvelhote commented at 7:50 am on May 18, 2017:

Arghh I didn’t know about this setmocktime. Didn’t think of searching either. Will get to it.

in test/functional/test_runner.py:113 in 2a57561973 outdated

109@@ -110,6 +110,7 @@
110     'rpcnamedargs.py',
111     'listsinceblock.py',
112     'p2p-leaktests.py',
113+    'server.py',

paveljanik commented at 6:24 am on May 18, 2017:

Please use the name uptime.py or so, not generic server.py.

rvelhote commented at 7:50 am on May 18, 2017:

I took as an example the net.py test file that referred to tests in rpc/net.cpp. Since the uptime command is in the server.cpp - although there are no tests for this file - I chose to create a server.py test file. Do you still think it’s best to use uptime.py?

paveljanik commented at 8:05 pm on May 18, 2017:

@rvelhote Yup, OK.

rvelhote commented at 7:33 pm on May 18, 2017: contributor

@paveljanik I made the change to setmocktime in the test and ~~held off on the name change of the test file until I get your final feedback after my reply to your comment.~~ renamed the testcase file for the uptime command.

Let me know when you think everything is in order and I will go ahead and squash the commits. Thank you for your help (and patience) in reviewing everything :)

paveljanik commented at 9:29 am on May 26, 2017: contributor

OK, let’s squash please. The travis failure is unrelated (actually fixed in #10429).

rvelhote force-pushed on Jun 5, 2017

in src/rpc/server.cpp:261 in 5ca8f57ea8 outdated

257@@ -258,6 +258,22 @@ UniValue stop(const JSONRPCRequest& jsonRequest)
258     return "Bitcoin server stopping";
259 }
260 
261+UniValue uptime(const JSONRPCRequest& jsonRequest)

jnewbery commented at 6:21 pm on June 8, 2017:

I think it makes more sense for this RPC to live in src/rpc/misc.cpp. I think server.cpp should be kept as lightweight and generic as possible.

rvelhote commented at 9:26 pm on June 8, 2017:

Thank you very much for your feedback John.

I placed it on src/rpc/server.php because the command was information related to the server rather than a utility command (although it can certainly be considered an utility command). That is my only argument for using server.cpp. Maybe let’s wait for other people’s opinion on this?

jnewbery commented at 9:44 pm on June 8, 2017:

Maybe let’s wait for other people’s opinion on this?

certainly. Just my opinion that this should live in misc.cpp. I’m happy to defer to the consensus opinion.

laanwj commented at 2:06 pm on June 19, 2017:

I understand both sides so don’t have a strong opinion. We want to have multiple JSON-RPC endpoints at some point, so the class should be reusable without adding a lot of clutter, but it seems harmless if they all export uptime.

However: server.cpp should be independent of bitcoin, so if you plan on keeping it there, please remove the explicit reference to Bitcoin below in the documentation.

in test/functional/uptime.py:1 in 5ca8f57ea8 outdated

0@@ -0,0 +1,32 @@
1+#!/usr/bin/env python3

jnewbery commented at 6:25 pm on June 8, 2017:

I’d prefer not to add a whole extra test for just this one command. I think it’d be better to move the uptime rpc into misc.cpp and then combine signmessages.py, rpcnamedargs.py and this new test into a new test file names misc.py.

However, that tidying can definitely be done in a follow-up PR.

in src/rpc/server.cpp:268 in 5ca8f57ea8 outdated

263+    if (jsonRequest.fHelp || jsonRequest.params.size() > 1)
264+        throw std::runtime_error(
265+                "uptime\n"
266+                        "\nReturns the total uptime of the Bitcoin server.\n"
267+                        "\nResult:\n"
268+                        "ttt        (numeric) The number of seconds that the Bitcoin server has been running\n"

jnewbery commented at 6:26 pm on June 8, 2017:

nit: I don’t really like ttt here. I think it’s fine just to have:

"\"time\" (numeric) The number of seconds that the Bitcoin server has been running\n"

rvelhote commented at 9:26 pm on June 8, 2017:

I used ttt because I saw it referenced in other commands for usage with time related values. For example in getpeerinfo you have conntime for a timestamp and also timeoffset for a time-based value in seconds. I think ttt will keep it consistent with other commands.

jnewbery commented at 9:45 pm on June 8, 2017:

sure. Again, just my personal preference. ttt is fine.

in src/util.h:33 in 5ca8f57ea8 outdated

28@@ -29,6 +29,9 @@
29 #include <boost/signals2/signal.hpp>
30 #include <boost/thread/exceptions.hpp>
31 
32+// Application startup time (used for uptime calculation)
33+static const int64_t nStartupTime = GetTime();

jnewbery commented at 7:06 pm on June 8, 2017:

This shouldn’t be static (a static variable in a header file means that any .cpp file that includes the header will have its own local variable called nStartupTime).

Instead, I think you should define this variable in util.cpp and then have an extern declaration here. See BITCOIN_CONF_FILENAME below for example.

I don’t know if you just copied the DEFAULT_LOG... constants below, but I believe those should also be defined in util.cpp.

rvelhote commented at 9:26 pm on June 8, 2017:

The DEFAULT_LOG constants are already in master. Might be something to improve then.

Also, I don’t know if you noticed, this variable was moved from src/qt/clientmodel.cpp to a more global area so that it can be reused by both the server and the QT client.

I will make this change as you suggested and force-push the commit.

jnewbery commented at 9:50 pm on June 8, 2017:

Yes, sorry I wasn’t clear - I didn’t expect you to change the DEFAULT_LOG constants. I do however think you need to move the nStartupTime definition to avoid the following build warnings:

 0In file included from addrman.h:14:0,
 1                 from addrdb.cpp:8:
 2util.h:33:22: warning: ‘nStartupTime’ defined but not used [-Wunused-variable]
 3 static const int64_t nStartupTime = GetTime();
 4                      ^   
 5  CXX      libbitcoin_server_a-bloom.o
 6In file included from addrman.h:14:0,
 7                 from addrman.cpp:6:
 8util.h:33:22: warning: ‘nStartupTime’ defined but not used [-Wunused-variable]
 9 static const int64_t nStartupTime = GetTime();
10                      ^   
11  CXX      libbitcoin_server_a-blockencodings.o
12 /bin/bash ./config.status
13In file included from bitcoind.cpp:18:0:
14util.h:33:22: warning: ‘nStartupTime’ defined but not used [-Wunused-variable]
15 static const int64_t nStartupTime = GetTime();
16                      ^
17...(repeated many times)

rvelhote commented at 10:20 pm on June 8, 2017:

No problem John, I understood what you meant :) When I mentioned I would make the change I was referring to moving nStartupTime to util.cpp. I pushed a new commit with this change.

jnewbery commented at 7:07 pm on June 8, 2017: member

A few nits. The only important one is nStartupTime shouldn’t be defined as static in the util.h header file.

rvelhote force-pushed on Jun 8, 2017

rvelhote commented at 8:47 am on June 19, 2017: contributor

@jnewbery @paveljanik When you have the opportunity, please let me know if you have any further feedback on this PR? The TravisCI build is failing but it’s because of #10429 and I have not updated my branch since I started the PR. Thank you.

paveljanik commented at 11:12 am on June 19, 2017: contributor

Needs rebase

in src/util.h:33 in 4e51e6daad outdated

28@@ -29,6 +29,9 @@
29 #include <boost/signals2/signal.hpp>
30 #include <boost/thread/exceptions.hpp>
31 
32+// Application startup time (used for uptime calculation)
33+extern const int64_t nStartupTime;

laanwj commented at 2:04 pm on June 19, 2017:

Please add a GetStartupTime function instead of exporting this variable.

rvelhote force-pushed on Jun 25, 2017

[RPC] Add an uptime command that displays the amount of time that bitcoind has been running c07475294a

rvelhote force-pushed on Jun 25, 2017

laanwj assigned laanwj on Jun 26, 2017

laanwj commented at 9:34 am on June 27, 2017: member

Tested ACK c074752

laanwj merged this on Jun 27, 2017

laanwj closed this on Jun 27, 2017

laanwj referenced this in commit 1680ee0edf on Jun 27, 2017

PastaPastaPasta referenced this in commit 9a0ab13f57 on Jul 6, 2019

PastaPastaPasta referenced this in commit 6476b20945 on Jul 8, 2019

PastaPastaPasta referenced this in commit bd58182315 on Jul 9, 2019

PastaPastaPasta referenced this in commit 41b3869031 on Jul 11, 2019

barrystyle referenced this in commit 6a45aa1918 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

[RPC] Add an uptime command that displays the amount of time (in seconds) bitcoind has been running #10400